search

arduino / ArduinoCore-API

Hardware independent layer of the Arduino cores defining the official API
https://www.arduino.cc/reference/en/
GNU Lesser General Public License v2.1
202 stars 118 forks source link

Remove guards as we already require C++11 #213

Closed jboynes closed 1 year ago

jboynes commented 1 year ago

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.

codecov-commenter commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (66aa7db) 95.52% compared to head (c9346ee) 95.52%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #213 +/- ## ======================================= Coverage 95.52% 95.52% ======================================= Files 16 16 Lines 1072 1072 ======================================= Hits 1024 1024 Misses 48 48 ``` | [Files Changed](https://app.codecov.io/gh/arduino/ArduinoCore-API/pull/213?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arduino) | Coverage Δ | | |---|---|---| | [api/String.cpp](https://app.codecov.io/gh/arduino/ArduinoCore-API/pull/213?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arduino#diff-YXBpL1N0cmluZy5jcHA=) | `97.27% <ø> (ø)` | | | [api/String.h](https://app.codecov.io/gh/arduino/ArduinoCore-API/pull/213?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arduino#diff-YXBpL1N0cmluZy5o) | `90.00% <ø> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

aentinger commented 1 year ago

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.

jboynes commented 1 year ago

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.

aentinger commented 1 year ago

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.

jboynes commented 1 year ago

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.

aentinger commented 1 year ago

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.