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

Project requires at least C++20 #80

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

autoantwort
Copy link

No description provided.

@camio
Copy link
Member

camio commented Nov 16, 2024

Thanks for the PR!

We haven't been using target_compile_features because doing so can lead to subtle ODR violations in large projects. See bemanproject/exemplar#40 (comment) for a more elaborated rationale.

@autoantwort
Copy link
Author

Hm, ideally the "large project" would also use cmake to links it stuff together and then the c++20 flag would get propagated to the final executable.
In vcpkg CMAKE_CXX_STANDARD don't gets set by default (the users can if they want), so it currently simply fails to build since the default one is c++11 for apple clang.

@camio
Copy link
Member

camio commented Nov 16, 2024

Hm, ideally the "large project" would also use cmake to links it stuff together and then the c++20 flag would get propagated to the final executable.

Large projects aren't built completely from source. Frequently, they incorporate shared libraries that come as binaries which exacerbates the ODR violation issues.

My understanding is that VCPkg interacts with CMake by providing a toolchain file. How does this toolchain file determine compiler flags?

@steve-downey
Copy link
Member

Changing the flags of a consumer is a great way to break the consumer, with the breakage varying between subtle and loud. Loud is good because it gets reverted quickly.
Even if I'm building all of my project from source, having a dependency make a huge change, like std level, can be a problem.
Saying "No" and producing an error might be better, but the compiler errors should be fairly clear even without that.

@ClausKlein
Copy link

I do not see that this may a problem for at least a header only library:

https://gitlab.kitware.com/cmake/cmake/-/issues/19136#note_1598283

@ClausKlein
Copy link

ClausKlein commented Dec 7, 2024

FYI: bemanproject/inplace_vector#33

This seems not a better way:

bash-5.2$ git ls-files ::*.cmake ::*CMakeLists.txt ::*.json | xargs egrep --color -rw '(CMAKE_CXX_EXTENSIONS|CMAKE_CXX_STANDARD\w*|std)' 
CMakePresets.json:        "CMAKE_CXX_EXTENSIONS": false,
CMakePresets.json:        "CMAKE_CXX_STANDARD": "20",
CMakePresets.json:        "CMAKE_CXX_STANDARD_REQUIRED": true,
etc/ci-clang-toolchain.cmake:    "-std=c++20 \
etc/clang-flags.cmake:set(CMAKE_CXX_STANDARD 20)
etc/gcc-flags.cmake:set(CMAKE_CXX_STANDARD 23)
etc/gcc-flags.cmake:set(CMAKE_CXX_FLAGS "-std=c++23 -Wall -Wextra " CACHE STRING "CXX_FLAGS" FORCE)
etc/gcc-toolchain.cmake:    "-std=c++20 \
etc/llvm-16-toolchain.cmake:    "-std=c++20 \
etc/llvm-master-toolchain.cmake:    "-std=c++2a \
etc/llvm-toolchain.cmake:    "-std=c++20 \
src/beman/optional26/tests/CMakeLists.txt:            "-DCMAKE_CXX_STANDARD=${CMAKE_CXX_STANDARD}"

and results in something like this: https://github.com/bemanproject/optional26/actions/runs/12210018831/job/34065577908

Copy link
Member

@neatudarius neatudarius left a comment

Choose a reason for hiding this comment

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

As per ps://github.com/bemanproject/beman/pull/76,

"Therefore, target_compile_features (e.g., cxx_std_20) must not be used
because it modifies the compilation environment of dependent targets. Compiler
support for required features should be determined at CMake configuration time
using check_cxx_source_compiles."

Please hold this PR until we finish updating the Beman Standard with this missing piece.

@ClausKlein
Copy link

ClausKlein commented Dec 12, 2024

The beman_project is partly on a realy wrong way,

please read https://crascit.com/professional-cmake/ before make suche a decision!

and before see https://discourse.bemanproject.org/t/c-20-modules-in-beman-project-libraies/279/5?u=clausklein

@camio
Copy link
Member

camio commented Dec 12, 2024

The beman_project is partly on a realy wrong way,

@ClausKlein, please review our code of conduct. This comment does not embody the friendliness, patience, and respect that we adhere to in these spaces.

please read https://crascit.com/professional-cmake/ before make suche a decision!

Telling someone to read a book is not a reasonable way to move a conversation forward.

@ClausKlein
Copy link

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.

5 participants