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

Add C++ language standard menu item #205

Closed
wants to merge 10 commits into from

Conversation

DrItanium
Copy link
Contributor

@DrItanium DrItanium commented Jan 26, 2021

Made it possible to change the C++ language standard from the default found in platform.txt to gnu++11 (C++11), gnu++14 (C++14), or gnu++17 (C++17) via the arduino ide tools menu.

Selecting Default means to not add any extra options to the C++ command lines.

EDIT: introduced two new variables:

  • compiler.cpp.languageStandard
  • compiler.c.languageStandard
    These default to gnu++11 and gnu11 for C++ and C respectively. The menu items overwrite the cpp language standard variable instead now.

avr/boards.txt Outdated
@@ -241,6 +242,21 @@ menu.bootloader=Bootloader
1284.menu.clock.1MHz_internal.build.f_cpu=1000000L


# C++ Language Standard
1284.menu.LanguageStandard.Default=Default
1284.menu.LanguageStandard.Default.compiler.cpp.extra_flags=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler.cpp.extra_flags property is already in use by the LTO board option:

1284.menu.LTO.Os.compiler.cpp.extra_flags=

Although it is possible to do this by using dedicated properties in each of the options and then defining compiler.cpp.extra_flags by referencing each of the dedicated properties (e.g., compiler.cpp.extra_flags={lto_flags} {std_flag}), the platform really shouldn't be using the compiler.cpp.extra_flags property at all, since it is intended to be left for the use of the platform user in order to allow them to customize the compilation command. If the platform authors uses these properties then when the user tries to use them their definition overrides the platform's definition of the property. The platform author has the power to use any arbitrary property name they like so they have no need to monopolize the user's properties.
Some related discussion on that subject here: arduino/arduino-cli#846

Copy link
Contributor Author

@DrItanium DrItanium Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know. I do not use LTO so I did not see the conflict / overwrite. I incorrectly assumed that compiler.cpp.extra_flags was unique to each menu item and was concatenated magically. After typing that previous sentence out, I realize how absurd that assumption was.

Would updating the platform.txt to have a compiler.cpp.languageStandard variable make more sense? While specific to the current problem, it would never run afoul of other places in the build process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good approach. The alternative would be to use a single property for all the board level extra flags for each compilation recipe. For example:
boards.txt

...
1284.menu.LTO.Os_flto.compiler.cpp.board_extra_flags.lto=-Wextra -flto -g
...
1284.menu.CppLanguageStandard.Cpp11.compiler.cpp.board_extra_flags.std=-std=gnu++11
...
1284.compiler.cpp.board_extra_flags={compiler.cpp.board_extra_flags.lto} {compiler.cpp.board_extra_flags.std}
...

platform.txt

compiler.cpp.flags=-c -g -Os {compiler.warning_flags} {compiler.cpp.board_extra_flags} -fno-exceptions -ffunction-sections -fdata-sections -fno-threadsafe-statics -MMD

But there's no functional different between the two approaches. It's purely a style choice, which would be for MCUdude to make. I'm only an interested 3rd party in this project.

Note that there is no magic to these particular property names. I just made something up at random for the sake of the example. They could just as well be named foobar. It can be a bit confusing because some property names have special treatment by the build system, but the platform author is also welcome to use arbitrary properties of any name they like as long as they don't collide with the build properties that have special treatment.

I realize how absurd that assumption was.

I don't think so. The platform system is pretty complex and the fact that it uses this custom "properties" data format makes it even more difficult to understand. Fortunately, the documentation has improved a bit since the time I learned it mostly through reverse engineering the existing platforms and a ton of trial and error.

Copy link
Contributor Author

@DrItanium DrItanium Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose compiler.cpp.languageStandard over compiler.cpp.board_extra_flags.std because I have to deal with many compilers at work which do not use -std= to specify the C++ language mode (--c++11, --eecpp, etc). Call it descriptive paranoia.

Regardless of the style chosen by MCUDude, I really want the ability to change the C++ language standard without having to manually modify platform.txt.

Thank you for letting me know that I was clobbering options :).

@MCUdude
Copy link
Owner

MCUdude commented Feb 3, 2021

Is there any reason why gnu++17 (C++17) can't be default? AFAIK it's backward compatible with older versions.

@DrItanium
Copy link
Contributor Author

DrItanium commented Feb 24, 2021

I can make it c++17 by default if you want. I was trying to be nice to others :)

I actually can't think of a reason why it couldn't be the default (I have been constantly modifying board packages for over a year to enable C++17 mode).

@DrItanium
Copy link
Contributor Author

I am sorry it took so long to reply, I did not get a notification from github via email.

I have gone ahead and made C++17 the default target.

@MCUdude
Copy link
Owner

MCUdude commented Feb 24, 2021

I was thinking about the "risks" of using c++17. Is it 100% backward compatible with older versions, like c++11 so it won't break any existing code? @per1234 is there a good reason why c++17 isn't default in the official Arduino AVR core?

@DrItanium
Copy link
Contributor Author

I deal with C++ and its various versions all the time at work. I have not encountered any "breaking" changes going from C++11 -> C++14 -> C++17. I haven't encountered a breaking change in the language it self. I have encountered changes (although not really breaking) with the STL relating to deprecation or removal (although that is well defined).

I could imagine that the reason why C++17 isn't default is that the version of the compiler may not support all of C++17's features. I cannot remember which version of GCC is being used. If it is gcc 7, then the missing features relate to the STL mostly (https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html#status.iso.2017). See https://gcc.gnu.org/projects/cxx-status.html#cxx17 for more information on C++17 language support based on compiler.

Now, with all of that being said. I have been forcing C++17 mode for at least a year on various AVR and ARM based projects. With ARM I do get access to the STL and even then it seems nearly complete. It has just worked well for me.

@per1234
Copy link
Contributor

per1234 commented Feb 24, 2021

is there a good reason why c++17 isn't default in the official Arduino AVR core?

Even though it's about C++14 and the SAMD platform, this conversation looks like it might contain some relevant information:
arduino/ArduinoCore-samd#313

@DrItanium
Copy link
Contributor Author

DrItanium commented Feb 24, 2021

So by forcing C++17 mode, there are extra delete operators that must be defined (those are the errors that travis is reporting). This was discussed in the conversation that @per1234 posted. It isn't a show stopper but this is an example of changes made in newer versions of C++.

EDIT: If you're super curious about this, https://en.cppreference.com/w/cpp/memory/new/operator_delete

@DrItanium
Copy link
Contributor Author

Sorry for the delay in fixing things, are there any other blockers?

@MCUdude
Copy link
Owner

MCUdude commented Apr 19, 2021

Sorry for the delay in fixing things, are there any other blockers?

Well, for a start I don't want a separate menu option for this, I'd rather stick with a fixed version, for instance, c++17.

Second, I have little or no idea how or if this will affect existing user programs? Does it break code that used to work with c++11?

Third, all changes to the core files have to be done in a separate repo: https://github.com/MCUdude/MCUdude_corefiles.
The MCUdude_corefiles is a subtree that gets pulled into this and a few other reports I have. Your changes to new.h/cpp should be committed to that repo, and only the platform.txt changed in this repo.

@DrItanium
Copy link
Contributor Author

DrItanium commented Apr 19, 2021

Responses to all three points plus what I think should be done:

First, I agree that I would just like to migrate to C++17 by default. I don't like having the menu option either. To be honest, one should just have access to C++17 language features without thinking about (constexpr-if is one of the big ones).

Second Point:

It should have very little impact on most programs that are written in C++11. C++17/C++14 generally enhance the features introduced in C++11 or build upon them. The only language feature I know that C++17 dropped is trigraphs which has been deprecated for some time.

In my personal observation, code which was accepted in C++11 but not C++17 usually revolves around Undefined Behavior (UB). If the compiler generates a warning then that means the program is stepping into the realm of UB. A good example is signed vs unsigned comparisons and how it is up to the compiler to resolve them.

In general, I have found C++17 to be C++14 but better and C++14 to be C++11 but better. The changes each version of the language introduces is significantly smaller than C++11. In fact, C++ releases new language versions on a three year cadence these days.

I think that it comes down to trying out your CI tests and see if we get any breakage, from what I can tell you have quite a large number of examples to pull from. I should also mention that MightyCore is not the only board support package I have hacked C++17 into. I've done it with the Adafruit SAMD, Arduino AVR, Arduino SAMD, and Adafruit AVR BSPs with zero failures. I do get extra warnings in some cases (the SAMD libraries complain about endianess) but no failures I've seen so far.

Point Three, I did not realize that the corefiles directory was a submodule... it has been a while. I think I should abandon this pull request just on the basis of this.

You have raised some really good points, because of them I think that it makes more sense to break this pull request apart into two separate pull requests:

Pull Request 1) Add the new memory operators introduced in C++14 to the MCUdude_corefiles repo.
Pull Request 2) Change the default language mode to gnu++17 platform.txt and see if your tests break at all (after the first pull request is merged).

Does this make sense?

@MCUdude
Copy link
Owner

MCUdude commented Apr 19, 2021

Thanks for your thoughts. I think we should give it a go and see if people complain (or even notice anything). The world moved forward after all...

Pull Request 1) Add the new memory operators introduced in C++14 to the MCUdude_corefiles repo.
Pull Request 2) Change the default language mode to gnu++17 platform.txt and see if your tests break at all (after the first pull request is merged).
Does this make sense?

It makes total sense. I'm waiting for your new PRs 👍

@MCUdude MCUdude closed this Apr 19, 2021
@DrItanium DrItanium deleted the language_mode_setting branch April 20, 2021 20:26
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.

3 participants