-
-
Notifications
You must be signed in to change notification settings - Fork 133
Remove guards as we already require C++11 #213
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
Conversation
codecov-commenter
commented
Sep 9, 2023
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@ ## master #213 +/- ## ======================================= Coverage 95.52% 95.52% ======================================= Files 16 16 Lines 1072 1072 ======================================= Hits 1024 1024 Misses 48 48
☔ View full report in Codecov by Sentry. |
Can you be more specific? The requirements of the test code is not relevant here, what's relevant is that all cores using ArduinoCore-API support C++11 in order to remove those guards.
I thought that was a requirement for using this API package.
There are several places in the production code that already use C++11 features unguarded e.g. line 72 in this file that uses a delegatng constructor, as does CanMsg.h. Other features used are auto and "commas in enum" in Common.h, "long long" in Print.h. Possibly more, the compiler gave up at that point.
Given the guards were only in String code and that not all use of C++11 features was being guarded I thought they were old dead code that could be cleaned up.
That's true. But we do now know how many or which cores (that use ArduinoCore-API) are out there that do not support advanced language features. If anything, those advanced C++ usage should be hidden behind the same kind of include guards.
We do know that all cores currently using ArduinoCore-API support C++11 because its features are already used, unguarded, in several places in the API code. That includes the arduino megaavr and samd cores.
Are there cores trying to adopt ArduinoCore-API that can't provide C++11 support?
The one that jumps to mind might be the arduino AVR core but my understanding is that's already using -std=gnu++11 so that wouldn't be an issue.
The issue is more about 3rd party cores, but a quick sampling of popular cores shows that Arduino_Core_STM32 uses C++17), arduino-esp32 uses C++17 and yes, even ArduinoCore-avr uses [C++11]. I therefore consider it safe to merge this issue, if a 3rd party core that already uses ArduinoCore-API want to upgrade to the next released version they may as well consider updating their tooling. Furthermore, as you've said it, the CAN API and other code sequences in ArduinoCore-API already require a minimum of C++11.
There are some legacy guards around the availability of C++11 features. However, we already require that version in this file as well as others so they can be removed.