Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: don't write to global variable CMAKE_CXX_STANDARD #754

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

OlivierLDff
Copy link
Contributor

@OlivierLDff OlivierLDff commented Sep 26, 2022

Instead use modern target_compile_features with cxx_std_11 (since this project is compatible with C++11 and later). More information: https://cmake.org/cmake/help/latest/command/target_compile_features.html

If user wants to access c++14/17/20 features, it is up to him to define cxx_std_ on his target. Or he can still use the old school CMAKE_CXX_STANDARD global variable.

This commit might break project relying on date writing to a global variable. If user links his target to date, his target will be promoted at least to c++11 if not specified otherwise.

Instead use modern `target_compile_features` with `cxx_std_11` (since this project is compatible with C++11 and later).
More information: https://cmake.org/cmake/help/latest/command/target_compile_features.html

If user wants to access c++14/17/20 features, it is up to him to define cxx_std_<version> on his target. Or he can still use the old school CMAKE_CXX_STANDARD global variable.

This commit might break project relying on date writing to a global variable. If user links his target to date, his target will be promoted at least to c++11 if not specified otherwise.
Copy link

@ChrisThrasher ChrisThrasher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! I recently did the same thing in fmtlib here. Don't forget you need to bump the minimum CMake version to 3.8 to ensure that this compile feature exists.

…ists.

I increase the minimum CMake version to 3.8 because that's when the cxx_std_11 compile feature was added. Realistically this requirement could be increased to at least 3.10 since 3.10 is what Ubuntu 18 ships with and it's unlikely that any users are locked to an older CMake version than that.

Text copied from fmtlib/fmt#3205
@OlivierLDff
Copy link
Contributor Author

I like this! I recently did the same thing in fmtlib here. Don't forget you need to bump the minimum CMake version to 3.8 to ensure that this compile feature exists.

Sure done. Have a nice day.

@Tachi107
Copy link
Contributor

Tachi107 commented Dec 21, 2022

If user wants to access c++14/17/20 features, it is up to him to define cxx_std_ on his target.

Unfortunately it is not that simple. If you compile libdate-tz in e.g. C++17 mode you cannot use it in projects targeting an older standard, as some functions are conditionally complied depending on whether std::string_view is available or not, like locate_zone(). The same applies the other way around; if you compile the .cpp file in C++11 mode and then use the headers in C++17 mode, you're code will try to link to the implementation that uses std::string_view, but the complied object only contains the std::string one so you'll get linking errors.

It might make sense to set cxx_std_17 as PUBLIC; that's what I'm planning to do in the Debian package.

Edit: alternatively, it might be better to make HAS_STRING_VIEW a public define.

@Tachi107 Tachi107 mentioned this pull request Dec 21, 2022
Tachi107 added a commit to Tachi107/howardhinnant-date that referenced this pull request Dec 21, 2022
…of the interface

If the library gets compiled with HAS_STRING_VIEW=1, consumers always
need to link to the functions using std::string_view, as they are the
only ones compiled into the shared library.

You can find a longer explanation here:
<HowardHinnant#754 (comment)>
@basilgello
Copy link
Contributor

Maybe a crazy idea but… Can we compile both sets of functions somehow to produce libraries with string_view and string? Then we could ship the CMake configutations for every combination of parameters. I did this for kissfft, it is time consuming but doable.

@Tachi107
Copy link
Contributor

Can we compile both sets of functions somehow to produce libraries with string_view and string?

Yes, it should be possible. But even then, the C++ version has to be set to cxx_std_17, otherwise the std::string_view variant cannot be possibly compiled into the library.

HowardHinnant pushed a commit that referenced this pull request Sep 28, 2024
…of the interface

If the library gets compiled with HAS_STRING_VIEW=1, consumers always
need to link to the functions using std::string_view, as they are the
only ones compiled into the shared library.

You can find a longer explanation here:
<#754 (comment)>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants