Use target_compile_features to set c++ standard flags

[bjude]

Directly setting the compiler flags can break downstream compilation, particularly with nvcc

Fixes an item in list 2 from #2189

[MikeGitb]

I've said this a couple of times now: I don't think cinder should try to automagically set the standard version. That should be left to the user. At most, it should tell cmake the minimal required standard version (cxx_std_14 on linux and cxx_std_17 on windows IIRC).

[MikeGitb]

Just to be clear: This is still better than the status quo

[bjude]

using cmakes compile features (like cxx_std_14) allows for this, cmake will take the maximum of what Cinder needs and what the user wants (provided the user uses the compile features on their cmake target too)

I've kept the logic that was in the .cmake file which was to set the maximum standard that the users compiler can handle. This is obviously not the best method, Cinder should set the level that it requires and let cmake override that with a later standard if the user desires. I didn't change the logic as I'm not sure exactly what standards cinder requires on various platforms

[MikeGitb]

My point is, cinder should not set the standard to c++17/20/23 just because the compiler supports it. But as you said, it is whats currently there and the change is certainly an improvment. One thing you could do while changing that code is to remove the c++11 fallback, as IIRC cinder no longer supports c++11 on any platform.

(provided the user uses the compile features on their cmake target too)

The imho proper method to specify the standard is setting CMAKE_CXX_STANDARD - either as part of the cmake toolchain file, the command line invocation or maybe the top level cmake file from the application that calls into the library CMLs. I'd be a much bigger fan of cxx_std_XX if it was a requirement and cmake would just error out if the requirement is not satisfied by the current toolchain (-settings). Butt well it is what it is

[bjude]

My point is, cinder should not set the standard to c++17/20/23 just because the compiler supports it. But as you said, it is whats currently there and the change is certainly an improvment. One thing you could do while changing that code is to remove the c++11 fallback, as IIRC cinder no longer supports c++11 on any platform.

Agreed, but changing that risks breaking peoples builds so should probably be the subject of a different PR with input from people who have some authority on what standards Cinder supports for various platforms (ie not me!)

The imho proper method to specify the standard is setting CMAKE_CXX_STANDARD - either as part of the cmake toolchain file, the command line invocation or maybe the top level cmake file from the application that calls into the library CMLs. I'd be a much bigger fan of cxx_std_XX if it was a requirement and cmake would just error out if the requirement is not satisfied by the current toolchain (-settings). Butt well it is what it is

I think you might have that backwards, CMAKE_CXX_STANDARD silently decays to a lower standard whereas using target_compile_features should emit an error if the compiler cannot fulfil that requirement. The right way to do it is to add a PUBLIC compile feature to the cinder target to specify the minimum standard required to build cinder. If the minimum standard required to build Cinder is different to the minimum required to use Cinder the that could be handled by an INTERFACE and PRIVATE compile feature instead of a PUBLIC one but i dont think thats necessary.

[MikeGitb]

I think you might have that backwards, CMAKE_CXX_STANDARD silently decays to a lower standard

Not when you set CMAKE_CXX_STANDARD_REQUIRED to ON.

And the reason, why I prefer CMAKE_CXX_STANDARD over cxx_std_XX is because the latter easily introduces unexpected standard version mismatches between different parts of your project: Let's say lib A needs 11, and the executable B that uses it needs 14. As a result lib A will be compiled with 11, but when the headers are used in B, they are interpreted in 14 which may or may not be compatible with the binary of A.

Also, in older cmake versions (I don't remember which) cxx_std_XX would actually downgrade the standard version to XX, even if e.g. the compiler default is newer, which can also be annoying when the lib only requires 11, but adds features when compiled with 14.

Anyway all this has little to do with the PR at hand. It is an incremental improvement that should get merged, even if I don't like the section as a whole.