-
-
Notifications
You must be signed in to change notification settings - Fork 714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Arduino compatibility with the STL (min and max macro) #399
Conversation
Replaced the min and max macros with std::min and std::max to allow compilation of bits/stl_algobase.h, a core component of many STL components such as std::function.
They did this in the ESP8266 core for Arduino and it caused breakage due to Arduino's traditional macro-based Examples:
Demonstration code that compiled before this change, but causes an error after: void setup() {
byte foo = 42;
byte bar = max(foo, 100);
}
void loop() {}
You can argue that the code that broke is bad code, but the impact of this change should at least be considered. |
Fixed styling Co-Authored-By: db-dropDatabase <noah@koontzs.com>
Created multi-type template for min and max.
Good point. I've replaced the |
This is a much needed PR for SAMD boards, I hope it will be merged soon. I tried the changes locally by pasting directly into my local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't get into a release fast enough as far as I'm concerned. I keep having to manually edit my Arduino.h file with these changes or my code will not build.
### New in v1.7.0 1. Sync with [Arduino WiFiNINA Library v1.7.0](https://github.com/arduino-libraries/WiFiNINA/releases/tag/1.7.0). See [Add 'downloadOTA' command to download OTA file and verify length/CRC](arduino-libraries/WiFiNINA#124) 2. Add Arduino SAMD Packages_Patches to fix Arduino SAMD compiler error when using STL. See [Improve Arduino compatibility with the STL (min and max macro)](arduino/ArduinoCore-samd#399)
Any ideas when this is going to be merged? Currently, I am having to manually patch each time I use an Arduino Zero based board. |
If I read the PR correctly, it now removes the Also, in the ArduinoCore-API repo, this is also fixed in a similar way, but with even more reliable macros for C. See arduino/ArduinoCore-API#85 for some discussion about this and other related changes. |
@matthijskooijman The |
They are, but they are now inside the
I'm not sure, at some point all of this code should just be replaced by the API versions (see arduino/ArduinoCore-API#95), but I'm not sure what the progress on that is. Also, I'm unsure if any such changes would still be merged here separetely, that's something for the Arduino devs to answer, I guess. |
As far as I can see your proposed changes have made it into ArduinoCore-API. Since #567 has been merged you should have your desired functionality. Please reopen if you've got a different opinion. |
@aentinger IIUC #567 only adds automated testing in preparation of merging ArduinoCore-API here, right? I don't see the API used in master yet? Regardless, I think this can be closed, as the PR as-is is not ideal and will probably not be merged with an upcoming -API merge. |
My bad, I linked the wrong PR. In fact, #560 which was merged just yesterday, brought |
Ah, I see that the files from ArduinoCore-API have been imported verbatim now, rather than in an |
Hi 👋 that's not correct. The API needs to be symlinked in into an |
Right, I didn't look close enough again. I see how it works now, thanks! |
Replaced the min and max macros with std::min and std::max to allow compilation of bits/stl_algobase.h, a core component of many STL components such as std::function.