Work with arduinostl #2
Conversation
matthijskooijman
force-pushed
the
work_with_arduinostl
branch
from
e61f3e4 to
94eb477
Compare
|
And at the right time, can you add example which use this library with ArduinoSTL? |
matthijskooijman
force-pushed
the
work_with_arduinostl
branch
from
94eb477 to
dbe4018
Compare
|
I forgot to post my test sketch yesterday, but please find it below now. I'm not sure whether this would be good for an actual example, though, as it just tries to cover some corner cases which would be weird in a real-world example. #if 1 // Either include ArxTypeTraits or standard system headers (setting this to 0 might break e.g. void_t below)
#include "ArxTypeTraits.h"
#else
#include <functional>
#include <limits>
#endif
#if 0 // Check with standard libray or just ArxTypeTraits
#if __AVR__
#include <ArduinoSTL.h>
#endif
#include <limits>
#else
// Allow using arx::std using the std:: name
//namespace std { using namespace arx::arx_std; }
#endif
// ArxTypeTraits handles these, but undef these in case we're not including ArxTypeTraits
#undef min
#undef max
// This breaks currently
//using namespace arx;
std::function<void()> foo = []() {};
// When using ArduinoSTL, this is *not* constexpr
int y = std::numeric_limits<int>::max();
// But this is :-D
constexpr int z = arx::arx_std::numeric_limits<int>::max();
// Check that void_t is available and not ambiguous
using x = std::void_t<int>;
void setup() {
Serial.begin(115200);
randomSeed(analogRead(0));
if (random(2)) {
foo = []() {
Serial.println("A");
};
} else {
new (&foo) std::function<void()>([]() {
Serial.println("B");
});
}
}
void loop() {
// put your main code here, to run repeatedly:
foo();
} |
|
Cool! I'll try to clean up the history a bit more before merging this, though. I'm also currently trying to compile this under cygwin, just to doublecheck whether that works. |
|
Ok, thanks! I'll check it again after the refactor :) I think ArxTypeTraits itself looks perfect, but compiling your example with ArduinoSTL (making this flag to 1) causes some errors. Though I have tried it with original one, do you use one of forks? |
|
Oh, right, provided you're testing on AVR, that's this issue: mike-matera/ArduinoSTL#56 It happens regardless of ArxTypeTraits, it's a conflict due to the Arduino core offering the |
|
Ah, it's cool, thanks! |
This is not normally a problem, but clang warns that MSVC might produce linker errors for this.
This is needed for WCHAR_MAX and similar macros. This was only a problem when ArxTypeTraits.h was included without stdint.h being included already (e.g. through Arduino.h usually), but include it just in case.
On some architectures, various C++98 and C++11 things were defined by this library. On other architectures, these are assumed to be provided from the system libraries and some headers are included to make sure they are actually available after including ArxTypeTraits. However, the list of included headers was not complete, so not all names normally defined might be available on these architectures. This has probably not shown up so far since the headers that were included might include other headers internally as well (e.g. on ESP32, `numeric_limits` was available even though `<limits>` was not included, but on SAMD this is not the case). This makes the list of includes complete, so all names should now be guaranteed to be available.
This moves these replacements into their own file for clarity and includes that file earlier in ArxTypeTraits.h already, before including standard C++ headers. This fixes compilation on platforms that *do* have libstdc++ but also define min/max macros. One example of such platform is SAMD, though that was not currently broken because there ArxTypeTraits defined its own functions rather than including libstdc++ (but that can now be changed). This has probably not shown up as a problem before, since the platforms where libstdc++ was used, probably did not define min/max macros (but min/max functions). However, this is not necessarily true and any platform can define such macros, so just do the replacement always (provided these macros are defined, of course).
This prevents the macros from creating issues when Arduino.h is not yet inluded but is included later.
This makes it easier to update the copy of this code embedded in the ArxContainer library, since now the entire file can be copied as-is. This commit changes the preprocessor guard that was around the the definition of initializer_list, to protect it from being double-defined by ArxContanier as well. It is now a standard include guard named after the file. This might cause problems when combined with ArxContainer, which should be solved as soon as ArxContainer also includes this same file.
These all refer to things inside the current std namespace, so no need to qualify them.
This declares new stuff in another namespace to prevent conflicts when standard system libraries are available even when we thought they were not. By importing the new arx::arx_std namespace *into* the std namespace, users of this library do not need to be updated, they can still reference using the std namespace as before (except that in case of a conflict, they get the standard library version rather than a compilation error). In addition, this imports all of std into arx::arx_std as well, so you can *also* explicitly reference namespace arx::arx_std for everything, except that in case of a conflict, you get the arx version rather than the standard library version (this is relevant when arx defines a newer version of some name than the standard libraries, such as the C++11 arx version of numeric_limits vs the C++98 ArduinoSTL version of it). Note that it would have been nicer to name this namespace arx::std, but when doing `using namespace arx`, which imports the `std` name, this creates a conflict with the global `std` namespace.
These were added to prevent conflicts with OpenFrameworks also defining some things in the std namespace. But now that we define things in our own namespace, this conflict can no longer occur and these guards can be removed.
Previously, on some specific architectures, an inline placement new was declared, and on others `<new.h>` was included. But `<new.h>` is a non-portable header available only in some Arduino architectures, while declaring our own placement new might conflict with standard libraries that also declare an inline version (such as uclibc++/ArduinoSTL). This commit changes the behaviour to always include `<new>` if it is available (assuming it is complete or at least has placement new, which seems to be the case for existing Arduino cores). If `<new>` is not available (which only seems to be the case for megaavr and older avr cores currently), just declare placement new inline. This has a small chance of conflict, but seems to be ok for avr and megaavr now.
This ensures that initializer_list is really only defined when not already available from the standard-named header. Unlike the other things defined by this library, initializer_list *must* be defined in std, importing it into std is not enough. This means that if it would already have been defined elsewhere, this will produce a conflict (unlike for the other things defined).
These platforms have libstdc++ available normally, so just use it. This has become possible with some recent changes that prevent conflicts with the libstdc++ implementation on those platforms.
matthijskooijman
force-pushed
the
work_with_arduinostl
branch
from
19ced70 to
81db076
Compare
Previously, on the (mega)AVR architectures, no standard c++ library was assumed to be available. Only on these architectures, controlled by the `ARX_TYPE_TRAITS_DISABLED` macro, all the C++98 and C++11 stuff would be defined. On all architectures (include (mega)AVR), the `__cplusplus` macro would be used to decide whether or not to define the C++14 and C++17 stuff. This is now made a lot more generic, where `TypeTraits.h` now defines a single macro with the C++ version supported by the standard library. In the other headers, this macro is used to decide what to define and what to include from the standard library. The C++98 and C++11 stuff is now split, so on e.g. AVR with ArduinoSTL the C++98 stuff can be included but the C++11 stuff still defined (which prevents defining stuff that is not needed). Determining the supported C++ version no longer relies on the Arduino architecture macros, but instead sees which library is available exactly. For gcc's libstdc++ and clang's libc++, the compiler `__cplusplus` version is assumed to be correct, for uclibc++ C++98 is assumed. Other standard libraries raise an error, since we have no idea about their capabilities. If no standard library is available (e.g. AVR without ArduinoSTL, the version macro is set to 0).
Even when the standard library might not support C++11 yet, the compiler should to allow things like variadic templates and other C++11 language constructs to be used.
matthijskooijman
force-pushed
the
work_with_arduinostl
branch
from
81db076 to
eba2f99
Compare
|
Ok, a couple of dozen rebases later and I think the history looks nice and clean now. I now did the namespace rename before the library-detection refactor, and it turns out the namespace rename was actually enough to make things work with ArduinoSTDL (but the refactor is still nice, because the code is now cleaner and only defines what is really needed). I also split off some smaller commits here and there and improved the commit messages, should be easy to review now. I also moved the I think that this PR is ready for another review and merging as far as I'm concerned :-) |
|
Here's a slightly updated version of the example, which also tests #include "ArxTypeTraits.h"
#if 0// Check with standard libray or just ArxTypeTraits
#if __AVR__
#include <ArduinoSTL.h>
#endif
#include <limits>
#endif
// Check this doesn't break
using namespace arx;
std::function<void()> foo = []() {};
// When using ArduinoSTL, this is *not* constexpr
int y = std::numeric_limits<int>::max();
// But this is :-D
constexpr int z = arx::arx_std::numeric_limits<int>::max();
// Check that void_t is available and not ambiguous
using x = std::void_t<int>;
// Check that initializer_list works
size_t listlen(std::initializer_list<int> s) { return s.size(); }
const size_t len = listlen({1, 2, 3});
void setup() {
Serial.begin(115200);
randomSeed(analogRead(0));
if (random(2)) {
foo = []() {
Serial.println("A");
};
} else {
new (&foo) std::function<void()>([]() {
Serial.println("B");
});
}
}
void loop() {
// put your main code here, to run repeatedly:
foo();
}
I compile-tested this on ESP32, SAMD and AVR. |
|
Thanks so much! I've checked this PR also in my several libraries which uses ArxTypeTraits. Currently only one problem is about ESP8266. ESP8266's |
|
Hm, it seems that ESP8266 still uses gcc 4.8, and However, in my local test, I get errors about redefinition of Lines 11 to 13 in eba2f99 Turns out the ESP8266 libc defines this macro manually: https://github.com/esp8266/Arduino/blob/cc042b99d1c966a8b3e92ca9b7ffb3b497d0af98/tools/sdk/libc/xtensa-lx106-elf/include/sys/cdefs.h#L79-L81 Which is a bit of a bummer, since to ArxTypeTraits work on ESP8266, |
Turns on ESP8266 does not support `__has_include`, but its libc defines a dummy version of it that always returns false, causing conflicts between system headers and ArxTypeTraits. This makes the following changes: - Replace uses of `__has_include` with a new `ARX_SYSTEM_HAS_INCLUDE` header (which defaults to just forwarding to `__has_include`). - On ESP8266, let `ARX_SYSTEM_HAS_INCLUDE` always return true (since we know that it supports the headers we check for). - On other platforms that use gcc < 5 (which does not have `__has_include` yet), flag an error (even if `__has_include` is defined, since it must then be a dummy version). - Move all this into its own header file, so it can be easily copied into ArxContainer along with "initializer_list.h".
|
Thanks, it looks good!
Ah yes, I also got about initializer_list and placement new. |
|
Done, see the latest commit. Compiletest on ESP8266, ESP32, SAMD and AVR (the last one with and without ArduinoSTL). |
|
After merging, I made some modifications I missed.
Please confirm them and let me know if you have any comments :) |
|
I left some inline comments on some of your issues. As for Due, your observation surprises me, since (just checked), it does have libstdc++. But when I use the same approach as for ESP8266 (let So that's a bit of a different problem. In fact, the problem seems to be that linking is done by |
|
Thanks for your comment, I've fixed those comments. And sorry when I thought Thanks again! |
|
If you can, can I ask you how you find which is the problem inside GCC (g++) like this?
I'm not confident about such details currently, but I want to be go deeper if I can. I could go to functional of arm-none-eabi 4.8.3 inside of sam directory, but I couldn't go further like you. How could you find that cause? If you can, let me know. (The only way is to read and understand codes inside such toolchains maybe...) Thanks, in advance. |
I first switched SAM from gcc 4 to gcc 7 (the one from SAMD). I used a bit of a hack, replacing the 4.8.something directory in |
|
Thank you, great. I should go more along with the compiler... This library was the result of my research to understand what the type_traits works (and how to write it), so I will go further with compiler by improving (and debugging) this library :) Thank you so much! |
|
For the Due, I filed a fix for the linker error here and suggested upgrading the compiler here. Some more minor things:
|
|
Ah, yes, it looks better. I will check if it works tomorrow (probably it works), and merge it.
Yeah, there is no "platform above"... I'm going to delete it.
I prefer |
See #1