Use recommended way to set the c++ standard via cmake #11
Conversation
CMakeLists.txt
Outdated
| if(NOT CMAKE_CXX_STANDARD MATCHES "17|20") | ||
| message(FATAL_ERROR "Unsupported C++ standard: ${CMAKE_CXX_STANDARD}") | ||
| endif() |
There was a problem hiding this comment.
Hi Thomas, thanks again. The comment here is the same as elsewhere. The default standard is fine, but the restriction should be avoided to allow for overrides at build time.
There was a problem hiding this comment.
Could you clarify what you mean by build time?
Can't you always do "CXXFLAGS=-std=c++23 make" to change this at build time?
Or do you mean at configure time during the cmake run?
There was a problem hiding this comment.
Hi @andresailer , yes, in this case, most particularly cmake configuration. You can set the standard with an additional flag, however, this conditional expression would then veto the use of, say, C++23, so I think this if block should be avoided.
There was a problem hiding this comment.
I mainly put this validation here to avoid some potential issues. We can adjust that to whatever you deem suitable (or remove it entirely). The main point with this series of pull requests was to switch to CMAKE_CXX_STANDARD for setting the c++ standard as that is the recommended way to do it (and is also what many build tools use to set it) instead of putting it into the CMAKE_CXX_FLAGS. Would you accept that simplified set of changes?
There was a problem hiding this comment.
Hi @tmadlener, yes, the updates to use CMAKE_CXX_STANDARD with a default of 17 are completely fine, it's really only the few lines highlighted in the comment that are problematic (plus the requested style change in the SDK PR), so if those are removed the remaining changes should be fine.
Use the recommended
CMAKE_CXX_STANDARDto set the c++ standard instead of adding it to theCMAKE_CXX_FLAGS. Leave the default at17and for now only allow17and20.Effectively the same change as PandoraPFA/PandoraSDK#16