Skip to content
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

Closed
wants to merge 6 commits into from

Conversation

prototypicalpro
Copy link

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.

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.
@per1234
Copy link
Contributor

per1234 commented Apr 11, 2019

They did this in the ESP8266 core for Arduino and it caused breakage due to Arduino's traditional macro-based min and max supporting parameters of different types, but std:min and std:max not supporting this.

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() {}
sketch_apr10a:3:26: error: no matching function for call to 'max(byte&, int)'

   byte bar = max(foo, 100);

                          ^

C:\Users\per\AppData\Local\Temp\arduino_modified_sketch_632562\sketch_apr10a.ino:3:26: note: candidates are:

In file included from C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.6.21\cores\arduino/Arduino.h:80:0,

                 from C:\Users\per\AppData\Local\Temp\arduino_build_865727\sketch\sketch_apr10a.ino.cpp:1:

c:\users\per\appdata\local\arduino15\packages\arduino\tools\arm-none-eabi-gcc\4.8.3-2014q1\arm-none-eabi\include\c++\4.8.3\bits\stl_algobase.h:260:5: note: template<class _Tp, class _Compare> const _Tp& std::max(const _Tp&, const _Tp&, _Compare)

     max(const _Tp& __a, const _Tp& __b, _Compare __comp)

     ^

c:\users\per\appdata\local\arduino15\packages\arduino\tools\arm-none-eabi-gcc\4.8.3-2014q1\arm-none-eabi\include\c++\4.8.3\bits\stl_algobase.h:260:5: note:   template argument deduction/substitution failed:

C:\Users\per\AppData\Local\Temp\arduino_modified_sketch_632562\sketch_apr10a.ino:3:26: note:   deduced conflicting types for parameter 'const _Tp' ('unsigned char' and 'int')

   byte bar = max(foo, 100);

                          ^

In file included from C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.6.21\cores\arduino/Arduino.h:80:0,

                 from C:\Users\per\AppData\Local\Temp\arduino_build_865727\sketch\sketch_apr10a.ino.cpp:1:

c:\users\per\appdata\local\arduino15\packages\arduino\tools\arm-none-eabi-gcc\4.8.3-2014q1\arm-none-eabi\include\c++\4.8.3\bits\stl_algobase.h:216:5: note: template<class _Tp> const _Tp& std::max(const _Tp&, const _Tp&)

     max(const _Tp& __a, const _Tp& __b)

     ^

c:\users\per\appdata\local\arduino15\packages\arduino\tools\arm-none-eabi-gcc\4.8.3-2014q1\arm-none-eabi\include\c++\4.8.3\bits\stl_algobase.h:216:5: note:   template argument deduction/substitution failed:

C:\Users\per\AppData\Local\Temp\arduino_modified_sketch_632562\sketch_apr10a.ino:3:26: note:   deduced conflicting types for parameter 'const _Tp' ('unsigned char' and 'int')

   byte bar = max(foo, 100);

                          ^

You can argue that the code that broke is bad code, but the impact of this change should at least be considered.

cores/arduino/Arduino.h Outdated Show resolved Hide resolved
cores/arduino/Arduino.h Outdated Show resolved Hide resolved
cores/arduino/Arduino.h Outdated Show resolved Hide resolved
per1234 and others added 2 commits April 11, 2019 08:32
Fixed styling

Co-Authored-By: db-dropDatabase <noah@koontzs.com>
Created multi-type template for min and max.
@prototypicalpro
Copy link
Author

Good point. I've replaced the using declarations with template functions supporting multiple types. These functions should behave the same as the min and max macro, but they can also coexist with std::min and std::max.

@NameOfTheDragon
Copy link

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 Arduino.h and it works very well. Without this change, basically any use of a library that uses std:: namespace is likely to fail due to collision between std::min() and std::max() and the #defines. I know you know this but from an end user perspective, I want you to know this is causing real pain that needs a fix.

@prototypicalpro prototypicalpro changed the title Improve Arduino compatibility with the STL Improve Arduino compatibility with the STL (min and max macro) Aug 9, 2019
@prototypicalpro
Copy link
Author

@per1234

Copy link

@NameOfTheDragon NameOfTheDragon left a 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.

khoih-prog added a commit to khoih-prog/WiFiNINA_Generic that referenced this pull request Aug 6, 2020
### 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)
@amirna2
Copy link

amirna2 commented Sep 30, 2020

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.

@matthijskooijman
Copy link
Collaborator

If I read the PR correctly, it now removes the abs and round macros when using C++ without providing alternatives?

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.

@prototypicalpro
Copy link
Author

prototypicalpro commented Oct 1, 2020

@matthijskooijman The abs and round macros are left unmodified (see https://github.com/arduino/ArduinoCore-samd/pull/399/files#diff-517f1981b375d8695337397cddc169d2R103-R104). I do agree that this problem has already been solved in other Arduino repositories, and has been for quite a while. I'd be happy to replace this PR with the changes in the ArduinoCore-API if that would improve the merge-ability?

@matthijskooijman
Copy link
Collaborator

The abs and round macros are left unmodified

They are, but they are now inside the #ifndef __cplusplus block, so not usable from C++ anymore.

I'd be happy to replace this PR with the changes in the ArduinoCore-API if that would improve the merge-ability?

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.

@aentinger
Copy link
Contributor

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 aentinger closed this Nov 26, 2020
@matthijskooijman
Copy link
Collaborator

@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.

@aentinger
Copy link
Contributor

My bad, I linked the wrong PR. In fact, #560 which was merged just yesterday, brought ArduinoCore-API to ArduinoCore-samd. Care to give it a spin?

@matthijskooijman
Copy link
Collaborator

Ah, I see that the files from ArduinoCore-API have been imported verbatim now, rather than in an api subdirectory was was previously suggested for another core (AVR maybe), that's why I didn't see it. Cool to see this is merged, I'll be sure to test this when I do some SAMD work again (lots of STM32 for me lately, though).

@aentinger
Copy link
Contributor

Ah, I see that the files from ArduinoCore-API have been imported verbatim now, rather than in an api subdirectory was was previously suggested for another core (AVR maybe), that's why I didn't see it.

Hi 👋 that's not correct. The API needs to be symlinked in into an api subdirectory. If you'd check out the core from master now it wouldn't compile if you don't symlink in the api. In the next packaged core release naturally the api folder will be available within the release, as requiring users to manually symlink would be a bit of a violation of Arduino philosophy.

@matthijskooijman
Copy link
Collaborator

Right, I didn't look close enough again. I see how it works now, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants