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

Update libenvpp version to 1.5, remove obsolete code #312

Merged
merged 2 commits into from
Nov 27, 2024
Merged

Conversation

PeterTh
Copy link
Contributor

@PeterTh PeterTh commented Nov 25, 2024

Very satisfying.
The new release of libenvpp does basic option parsing for us.
Also removed 3 completely unused functions from utils, and simplified some of the rest thanks to our switch to C++20.

Drive-by:

#include "version.h" // required for CELERITY_TRACY_SUPPORT

in tracy.h to document this unusual header ordering.

Copy link

Check-perf-impact results: (32e33781a20e2a3976ece8f7dc81ae2b)

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

github-actions[bot]

This comment was marked as outdated.

@coveralls
Copy link

coveralls commented Nov 26, 2024

Pull Request Test Coverage Report for Build 12047961487

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 94.913%

Totals Coverage Status
Change from base Build 12047884020: -0.04%
Covered Lines: 7115
Relevant Lines: 7233

💛 - Coveralls

Copy link
Contributor

@fknorr fknorr left a comment

Choose a reason for hiding this comment

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

LGTM, nice to get rid of that boilerplate!

include/utils.h Show resolved Hide resolved
@@ -141,7 +141,7 @@ macro(fetch_content_from_submodule DEPNAME RELPATH)
# - another depencency of celerity-runtime that it transitively shares a depencency with (SimSYCL -> libenvpp)
# The top-level CMake project must ensure that there are no conflicts between dependency versions in this case.
if(${DEPNAME}_FOUND)
message(STATUS "Using system ${DEPNAME} in ${${DEPNAME}_DIR}")
message(STATUS "Using existing ${DEPNAME} in ${${DEPNAME}_DIR}, please ensure that the version (${${DEPNAME}_VERSION}) is compatible")
Copy link
Member

Choose a reason for hiding this comment

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

I don't want to be overly pedantic, but IMO this change would better fit with the libenvpp bump!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're overly pedantic :P

@PeterTh PeterTh merged commit f2a0a76 into master Nov 27, 2024
17 checks passed
@fknorr fknorr deleted the libenvpp-update branch November 27, 2024 14:48
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