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

Add infrastructure for working with concepts #631

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

stephenswat
Copy link
Member

This commit adds the TRACCC_CONSTRAINT macro which acts as a concept in C++20 builds, and simply acts as the typename keyword in builds with older standards. It also adds the TRACCC_ENFORCE_CONCEPTS CMake flag which will throw an error if concepts are not available.

@stephenswat stephenswat added feature New feature or request build This relates to the build system labels Jun 26, 2024
@stephenswat stephenswat force-pushed the feat/concepts_base branch 5 times, most recently from 1423328 to 8b5666e Compare June 26, 2024 17:51
@stephenswat
Copy link
Member Author

Okay it took some faffing about because of CMake but this is ready to go.

@stephenswat stephenswat force-pushed the feat/concepts_base branch 2 times, most recently from 53213c8 to f91bc81 Compare June 26, 2024 18:15
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I do not like the change to ci_setup.sh. It should not be in the business of making decisions about compiler flags like that. 😦

I remember that you found that older CMake versions don't respect CMAKE_CUDA_STANDARD as they should. But then, let's do something similar to what we settled on in acts-project/acts#3318 with @paulgessinger. That the CI configuration YAML would specify some flags explicitly, using CMAKE_CUDA_FLAGS.

Yes, it's a bit fragile, but what's going on in this PR is quite fragile as well.

core/include/traccc/definitions/concepts.hpp Outdated Show resolved Hide resolved
@stephenswat
Copy link
Member Author

stephenswat commented Jun 28, 2024

Setting CMAKE_CUDA_FLAGS is high-key bad because it causes CMake to – silently, of course – ignore whatever is set in the CUDAFLAGS environment variable though... It's strictly more fragile than this approach as far as I am concerned but okay, I'll change it.

Edit: it also causes CMake to ignore it's own toolchain-specific default flags.

This commit adds the `TRACCC_CONSTRAINT` macro which acts as a concept
in C++20 builds, and simply acts as the `typename` keyword in builds
with older standards. It also adds the `TRACCC_ENFORCE_CONCEPTS` CMake
flag which will throw an error if concepts are not available.
@stephenswat
Copy link
Member Author

Updated!

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Unfortunately you are correct that setting CMAKE_CUDA_FLAGS makes the configuration ignore the $CUDAFLAGS environment variable. 😦 Which can indeed cause some problems. 😦

I believe that in the single job in which we need to force -std=c++20 on CUDA, it's fine to have $CUDAFLAGS ignored. So, just since this code setup is simpler, let's go with this anyway.

P.S. We will need to test C++20 with SYCL as well later on, but it's fine not to worry about that in this PR.

@stephenswat stephenswat merged commit f970f15 into acts-project:main Jun 28, 2024
26 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build This relates to the build system feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants