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

fix: forward compatibility with Acts-v31 #1188

Merged
merged 4 commits into from
Dec 22, 2023
Merged

fix: forward compatibility with Acts-v31 #1188

merged 4 commits into from
Dec 22, 2023

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Dec 21, 2023

Briefly, what does this PR introduce?

Thanks to C++20 we can now do templated lambdas which don't require syntactic validity of all code paths. That makes forward/backward compatibility across Acts versions easier with constexpr lambdas.

Ifdefs on defined Acts major versions allow us to bypass code blocks that are not compilable or parsable under different versions of Acts.

Ref: acts-project/acts#2649

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue: Acts v31 compatibility)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

No.

Thanks to C++20 we can now do templated lambdas which don't require
syntactic validity of all code paths. That makes forward/backward
compatibility across Acts versions easier with constexpr lambdas.
@wdconinc wdconinc requested review from veprbl and osbornjd December 21, 2023 21:23
@github-actions github-actions bot added the topic: tracking Relates to tracking reconstruction label Dec 21, 2023
@wdconinc
Copy link
Contributor Author

Ha, gcc and clang disagree on the validity here. That's annoying. Probably https://stackoverflow.com/questions/55909018/an-if-constexpr-branch-does-not-get-discarded-inside-lambda-that-is-inside-a-t and some careful parsing of the standard language...

@wdconinc
Copy link
Contributor Author

This is one of the remaining two red boxes for clang's c++20 support, as shown at https://en.cppreference.com/w/cpp/compiler_support#cpp20 (it is P0588R1).

Copy link
Contributor

@osbornjd osbornjd left a comment

Choose a reason for hiding this comment

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

What exactly does this get us? One less std::move for when we update to v31, after which we'll just delete the #else?

@wdconinc
Copy link
Contributor Author

The current code does not compile under v31 because some of these config objects are now required to be unique ptrs and need explicit move semantics (and under v30 the move semantics doesn't work because it's a call by reference that we can't move to).

In a world where we make more frequent Acts upgrades we can add the backwards compatibility support gradually and support at least compilation under more than just one version. It allows us to roll out new versions in a more user-friendly way: we can build a container with Acts v31 that has the same main version that is in the container with Acts v30.

@wdconinc
Copy link
Contributor Author

We can keep the v30 support until it is not maintainable anymore. At this point I imagine we might have a hard break at v32 again due to the changes in the dd4hep geometry conversion that are being merged now. We probably won't want to do ifdefs for large parts of the code base that would be required to support both sides of that transition.

@veprbl
Copy link
Member

veprbl commented Dec 22, 2023

I don't get why we need ifdef. I think xvalue implicitly casts to rvalue.

@wdconinc
Copy link
Contributor Author

Of course I tried that too as the very first thing...

@wdconinc
Copy link
Contributor Author

But I'd be much happier with a clean solution where the same code works for both, so please have at it!

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

This compiles with acts 31.2.0

@wdconinc
Copy link
Contributor Author

Thanks for reducing the amount of ifdefed code!

@wdconinc wdconinc added this pull request to the merge queue Dec 22, 2023
Merged via the queue into main with commit 03b1049 Dec 22, 2023
73 checks passed
@wdconinc wdconinc deleted the acts-v31 branch December 22, 2023 17:13
wdconinc added a commit that referenced this pull request Dec 23, 2023
### Briefly, what does this PR introduce?
~~Thanks to C++20 we can now do templated lambdas which don't require
syntactic validity of all code paths. That makes forward/backward
compatibility across Acts versions easier with constexpr lambdas.~~

Ifdefs on defined Acts major versions allow us to bypass code blocks
that are not compilable or parsable under different versions of Acts.

Ref: acts-project/acts#2649

### What kind of change does this PR introduce?
- [ ] Bug fix (issue #__)
- [x] New feature (issue: Acts v31 compatibility)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
No.

---------

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: tracking Relates to tracking reconstruction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants