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

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

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

jboynes
Copy link
Contributor

@jboynes jboynes commented Sep 9, 2023

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
Copy link

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
@@           Coverage Diff           @@
##           master     #213   +/-   ##
=======================================
  Coverage   95.52%   95.52%           
=======================================
  Files          16       16           
  Lines        1072     1072           
=======================================
  Hits         1024     1024           
  Misses         48       48           
Files Changed Coverage Δ
api/String.cpp 97.27% <ø> (ø)
api/String.h 90.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aentinger
Copy link
Contributor

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
Copy link
Contributor Author

jboynes commented Sep 11, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

jboynes commented Sep 11, 2023

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
Copy link
Contributor

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.

@aentinger aentinger merged commit 6cd8d8a into arduino:master Sep 13, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants