microsoft / GSL

Guidelines Support Library
Other
6.13k stars 737 forks source link

Raise CMake min to 3.8 #1061

Closed jpr42 closed 1 year ago

jpr42 commented 1 year ago

This avoids propogating -std=c++14 as a compile option

Currently if you use less than CMake 3.8 and you install the project -std=c++14 gets added to the INTERFACE_COMPILE_OPTIONS. This forces users to use C++ 14 or remove the property from the imported target.

The solution is to raise the minimum and use cxx_std_14

dmitrykobets-msft commented 1 year ago

Hi @jpr42

This forces users to use C++ 14 or remove the property from the imported target

Won't the -std=c++14 flag be overridden by whichever c++ version the user project specifies?

As currently implemented, the logic in gsl_set_default_cxx_standard will call CHECK_CXX_COMPILER_FLAG will produce a cmake warning if the compiler does not support C++14. With this PR this check is removed.

jpr42 commented 1 year ago

Won't the -std=c++14 flag be overridden by whichever c++ version the user project specifies?

I wasn't sure so I just tested this. Here is my summarized output from ninja.

/usr/bin/c++ ... -std=c++14 ... -std=c++17 GSL/tests/span_ext_tests.cpp

With compilers it's generally the last flag wins approach. And gcc doesn't seem to complain.

But 2 things.

jpr42 commented 1 year ago

As currently implemented, the logic in gsl_set_default_cxx_standard will call CHECK_CXX_COMPILER_FLAG will produce a cmake warning if the compiler does not support C++14. With this PR this check is removed.

If you use less the CMake 3.8 that is correct. Otherwise target_compile_features(GSL INTERFACE "cxx_std_14") handles that.

jpr42 commented 1 year ago

@dmitrykobets-msft as a test to see if CI would succeed I raised the minimum to 3.8

It fixes the issues you pointed out in developers workflows very cleanly.

Given that CMake 3.8 was released in April 10, 2017 (https://www.kitware.com/cmake-3-8-0-available-for-download/) and the latest release is 3.25 I think raising the minimum is agreeable.

If this change is unacceptable, I will revert my latest commit and fix the code as you suggested for compatibility with 3.1.

dmitrykobets-msft commented 1 year ago

@jpr42 sure, increasing to 3.8 globally instead of just for tests will indeed simplify things. If you can update the PR description I can approve these changes.

jpr42 commented 1 year ago

Updated PR description and squashed commits into 1