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

Move preprocessor definitions to configuration header #277

Merged
merged 2 commits into from
Aug 10, 2024
Merged

Conversation

fknorr
Copy link
Contributor

@fknorr fknorr commented Aug 9, 2024

Celerity's list of compile-time feature flags and options has grown considerably over time, and moving them from the command line to a generated header file will keep them more manageable.

Unfortunately we already have the canonical name config.h in use for the run-time configuration, so instead of causing confusion with a second file of the same name, I've chosen to move them into our existing generated header version.h. Not perfect for discoerability, but still semantically meaningful as that file enumerates all config options for the particular version of Celerity that's being built.

This PR also replaces the somewhat ad-hoc SYCL impl detection macros with a uniform set of CELERITY_SYCL_IS_* defines.

Also I've made sure all of our own defines are either 0 or 1, never undefined, and added -Wundef - this should avoid all future confusion around #if vs #if defined .

@fknorr fknorr requested review from psalz and facuMH August 9, 2024 20:24
Copy link

github-actions bot commented Aug 9, 2024

Check-perf-impact results: (5b2139f73fb4c21b4bcab6e559cd8c5a)

❓ No new benchmark data submitted. ❓
Please re-run the microbenchmarks and include the results if your commit could potentially affect performance.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10325268562

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 95.097%

Totals Coverage Status
Change from base Build 10325124557: 0.01%
Covered Lines: 6624
Relevant Lines: 6722

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10325210986

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 95.097%

Totals Coverage Status
Change from base Build 10325124557: 0.01%
Covered Lines: 6624
Relevant Lines: 6722

💛 - Coveralls

@fknorr fknorr requested review from PeterTh and removed request for facuMH August 9, 2024 21:04
include/version.h.in Show resolved Hide resolved
include/version.h.in Show resolved Hide resolved
@fknorr fknorr merged commit 501f165 into master Aug 10, 2024
34 checks passed
@fknorr fknorr deleted the config-header branch August 10, 2024 20:12
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