-
Notifications
You must be signed in to change notification settings - Fork 183
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
Conversation
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= |
There was a problem hiding this comment.
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:
Line 114 in 640d21a
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :).
Is there any reason why gnu++17 (C++17) can't be default? AFAIK it's backward compatible with older versions. |
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). |
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. |
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? |
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. |
Even though it's about C++14 and the SAMD platform, this conversation looks like it might contain some relevant information: |
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 |
Blink doesn't care about the extra delete operators, Blink_with_Timer0 does!
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. |
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. Does this make sense? |
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...
It makes total sense. I'm waiting for your new PRs 👍 |
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.