Conversation
|
might have an positive effect on comment pull/2626. To be checked with CI or someone with MacOS. |
| set(CMAKE_CXX_STANDARD 14) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
| set(CMAKE_CXX_EXTENSIONS OFF) | ||
|
|
There was a problem hiding this comment.
// line 11: assigned standard to 14
// line 12 : defined need
|
It's intentional that we don't pass any |
May I ask if there is a specific reason to prevent setting a specific standard to the compiler?
In general, the contributor might not know what the compiler(s) of ninja's baseline operating system is. Specifying the c++ standard helps to give a more unique behaviour across different compilers. Currently the contributor may use features from c++17 or higher without getting compile errors from its local compiler. This was the main reason for creating this merge request. |
|
Not compiling on RHEL 8 is already "wrong". It's correct that passing -std=c++14 gives you a "less wrong" result, but it would still be wrong (e.g. if there's a compiler bug in RHEL 8 we would care and not accept the PR). So a contributor always has to make sure that CI passes. If he wants to have the same environment as CI, he should work in Docker instead. |
|
Okay let me add some actual benefit: This way we don't have to update any compiler flags when we update our CI baseline OS. The intent isn't to have C++14, the intent is to have it easily compile on RHEL 8, macOS and Windows. If they would release an update that bumps the C++ standard to C++20? We would accept that. |
The c++ standard is not set to a unique version. Below one compile command on my linux with gcc 13.3.
current sources compiled:
/usr/bin/c++ -DUSE_PPOLL=1 -O3 -DNDEBUG -flto=auto -fno-fat-lto-objects -fdiagnostics-color=always -Wno-deprecated -MD -MT CMakeFiles/libninja.dir/src/elide_middle.cc.o -MF CMakeFiles/libninja.dir/src/elide_middle.cc.o.d -o CMakeFiles/libninja.dir/src/elide_middle.cc.o -c root/src/elide_middle.ccthis pull request gives:
/usr/bin/c++ -DUSE_PPOLL=1 -O3 -DNDEBUG -std=c++14 -flto=auto -fno-fat-lto-objects -fdiagnostics-color=always -Wno-deprecated -MD -MT CMakeFiles/libninja.dir/src/json.cc.o -MF CMakeFiles/libninja.dir/src/json.cc.o.d -o CMakeFiles/libninja.dir/src/json.cc.o -c root/src/json.ccThe noticable difference is the option
-std=c++14.My compiler does use a higher standard than c++14. This pull request will force the compiler to use c++14.