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

Consider making CMAKE_CXX_STANDARD a required user-provided option #6767

Closed
devjgm opened this issue Jun 14, 2021 · 6 comments · Fixed by #8537
Closed

Consider making CMAKE_CXX_STANDARD a required user-provided option #6767

devjgm opened this issue Jun 14, 2021 · 6 comments · Fixed by #8537
Assignees
Labels
type: question Request for information or clarification. Not an issue.

Comments

@devjgm
Copy link
Contributor

devjgm commented Jun 14, 2021

Today our CMake builds set the C++ language standard to 11 by default

set(CMAKE_CXX_STANDARD
11
CACHE STRING "Configure the C++ standard version for all targets.")

Users can override that with something like cmake -DCMAKE_CXX_STANDARD=17 ..., but if they don't specify something, we choose 11. Defaulting to C++11 will bias our telemetry data about C++ standard usage. And 11 may not be the correct default if users are using, say, C++17 in their own code (though it should work correctly in most cases).

We should consider not defaulting CMAKE_CXX_STANDARD to 11 and instead requiring that users explicitly specify the standard that they want to use.

Of course, we should continue to support C++11, and we'd need to have some builds that verify that our code works in a C++11 environment.

@devjgm devjgm added the type: question Request for information or clarification. Not an issue. label Jun 14, 2021
@ghost
Copy link

ghost commented Jun 14, 2021

Defaulting to C++11 will bias our telemetry data about C++ standard usage.

Drive-by opinion. If the primary motivation is telemetry, I feel like there's still a problem. If users have to explicitly specify it and they say copy/paste from the instructions, they might just leave -DCMAKE_CXX_STANDARD=11 in which as the same effect as it being default. Those using C++17 will explicitly change it but the numbers might look similar to before (this is just my intuition, I'm probably wrong). How about a big fat CMake warning/info that says "The default standard is C++11. If this is not what was intended, please change it..."

@devjgm
Copy link
Contributor Author

devjgm commented Jun 14, 2021

Well, I wouldn't say the primary motivation is telemetry. But to the extent that we have telemetry, making it more accurate would be good.

Maybe our instructions could say -DCMAKE_CXX_STANDARD=<select one of 11, 14, 17, or 20> or something, so that users can't copy-n-paste our chosen default.

Alternatively, maybe we should simply make our default the newest supported C++ standard, and then users can explicitly set it to anything they want all the way back to 11... but the default would be 20 or 17 or the latest standard supported by the compiler we're testing on.

@devjgm
Copy link
Contributor Author

devjgm commented Jun 15, 2021

Today we default to the oldest C++ standard we support (C++11), as described in the first comment in this issue. I'm now starting to like the idea of defaulting to the newest C++ standard available, while of course still supporting users who explicitly need an older standard (back to C++11). What might this actually look like?

Option 1: Default CMAKE_CXX_STANDARD in our CMakeLists.txt to the newest standard available.

How would we do this? I'm not totally sure, but I suspect we could use check_cxx_compiler_flag to see if flags like -std=c++17, etc. are available, and we could choose the newest one as the default value. I'm only assuming this is possible. If it's not, then clearly we can't do this option.

Option 2: No default for CMAKE_CXX_STANDARD; Require users to explicitly specify it

We would simply require that users specify -DCMAKE_CXX_STANDARD=XX on the command line. Our sample documentation that shows how to compile and install the code would use the newest available standard for the compiler/platform being shown, so that again if users simply copy-n-paste code, they're going to use a newish standard. I think this would help avoid the concerns that @remyabel 's mentioned above in #6767 (comment).

@devjgm devjgm mentioned this issue Oct 28, 2021
3 tasks
@coryan
Copy link
Contributor

coryan commented Dec 2, 2021

Here is an alternative thought: don't require the setting at all. For all the compilers we support (GCC >= 6.x, Clang >= 6.x, MSCV >= 2017) the default is C++14. If we (or the customer) left the standard undefined, we get what we want. We can check that it is not less than 11, but we do not need to require it.

@devjgm
Copy link
Contributor Author

devjgm commented Dec 2, 2021

I like that option a lot.

@devjgm
Copy link
Contributor Author

devjgm commented Mar 9, 2022

Surfacing this again. I really like Carlos's suggestion in his last post: we stop setting CMAKE_CXX_STANDARD and let customers set it if they want to, or use the compiler's default.

I may work on a PR for this. If someone sees problems w/ this, speak up.

@devjgm devjgm self-assigned this Mar 9, 2022
devjgm added a commit that referenced this issue Mar 15, 2022
Fixes: #6767

Rather than us explicitly setting (defaulting) CMAKE_CXX_STANDARD=11, we leave it unset and rely on the compiler's default C++ language standard. This will let customers more easily build w/ the "correct" standard for their platform.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: question Request for information or clarification. Not an issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants