cmake: Move minimum C++ standard to 20#1957
Conversation
rhornung67
left a comment
There was a problem hiding this comment.
@johnbowen42 we cannot force C++20 yet since it that would break some of our WSC user codes.
rhornung67
left a comment
There was a problem hiding this comment.
Let's hold off on merging this and reassess after the release.
rhornung67
left a comment
There was a problem hiding this comment.
Trying to dismiss approval
|
Umpire has a similar PR - we can reassess after the holidays on when we want to start enforcing c++20 minimum requirement |
f906f76 to
76ec8df
Compare
7005ddc to
475accc
Compare
|
@llnl/raja-core can I get a review on this PR now that we are ready to move forward with C++ 20? |
| ("${BLT_CXX_STD}" STREQUAL "c++11") OR | ||
| ("${BLT_CXX_STD}" STREQUAL "c++14")) | ||
| message(FATAL_ERROR "RAJA requires minimum C++ standard of c++17") | ||
| ("${BLT_CXX_STD}" STREQUAL "c++14") OR |
There was a problem hiding this comment.
Is there a way to do this in CMake with a '<=' type syntax? Meaning a single statement that says the equivalent of std <= 20.
Or, should we move to something like this?
set(CMAKE_CXX_STANDARD 20) set(CMAKE_CXX_STANDARD_REQUIRED ON)
That probably conflicts with what BLT does or may introduce an ordering dependency based on where the standard is set?!?
There was a problem hiding this comment.
I think the CMake way of specifying the minimum standard is the following (this was suggested in the CMake class offered to LLNL last year):
target_compile_features(RAJA PUBLIC cxx_std_20)
The PUBLIC means that downstream targets that include RAJA also need to have a minimum c++ standard of c++20.
There was a problem hiding this comment.
That could require a bit of cleverness. 98 (1998) will mess up simple extract year and LESS_EQUAL compare.
This would be a bit shorter than the OR sequence.
set(invalid_standards c++98 c++11 c++14 C++17)
if(BLT_CXX_STD IN_LIST invalid_standards)
There was a problem hiding this comment.
Thanks @adayton1 I did not know that was the cmake idiomatic way.
There was a problem hiding this comment.
FWIW, we originally had it written using the compile features check, but I ended up adding the extra logic because we regularly had issues with cmake claiming a given compiler didn't support a version when we knew it did. If that problem has gone away, then we should go back to doing it the idiomatic way.
There was a problem hiding this comment.
I would prefer it to fail if the version is too low.
There was a problem hiding this comment.
Another thing to be aware is that if we use cxx_std_20 as a PUBLIC target feature, downstream libraries will also be affected - meaning if they set the standard too low on their project, that will also be ignored and silently updated to c++20. So now that we've talked this through, I think it cxx_std_20 should either not be used or set to PRIVATE so that it only affects RAJA and not downstream projects.
There was a problem hiding this comment.
Another vote for failing on version too low. Silent magic is bad.
There was a problem hiding this comment.
Ditto for me -- fail when unsupported version is specified.
There was a problem hiding this comment.
Same here, fail when unsupported version is specified, ability to specify via BLT_CXX_STD.
Also, there have been efforts from folks like Daniel Taller to be able to enforce cxx standard via Spack. If the cxx standard specified as a variant in a spack spec actually resulted in another standard being use, that would be calling for troubles.
rhornung67
left a comment
There was a problem hiding this comment.
Remove the c++ std from corona sycl host-config
c9b305b to
6c54ad7
Compare
This pull request changes the minimum C++ version in CMake and build scripts to C++20. We probably want to hold off on merging this change until after the upcoming release is cut.