feat!: Use strong types for context objects #673
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Move away from using a raw typedef to std::any. Doing that has certain drawbacks: std::any can be convert constructed from anything. This causes problems when constructing anything that only accepts a context any. GCC9 even has recursion trouble in the type_traits infrastructure leading to compile time failures. Moving to wrappers can alleviate this. They only have an explicit constructor, meaning that they are not autoconstructed by conversion. This is a little bit more cumbersome to use, but much more robust I believe. Performance should not be affected.
paulgessinger
changed the title
Codecov Report
@@ Coverage Diff @@
## master #673 +/- ##
==========================================
+ Coverage 49.06% 49.08% +0.01%
==========================================
Files 329 330 +1
Lines 16438 16443 +5
Branches 7657 7657
==========================================
+ Hits 8066 8071 +5
Misses 2998 2998
Partials 5374 5374
Continue to review full report at Codecov.
|
asalzburger
reviewed
Jan 27, 2021
asalzburger
approved these changes
Jan 27, 2021
|
I approve, nice work Paul! Let's wait for the CI and merge then. |
|
I think I'm actually going to implement @benjaminhuth's suggestion before we merge this. |
|
Ok, I switched to an access method, and don't expose the |
asalzburger
reviewed
Jan 28, 2021
| @@ -84,7 +84,7 @@ inline const Acts::Transform3& AlignedDetectorElement::transform( | |||
| // Check if a different transform than the nominal exists | |||
| if (m_alignedTransforms.size()) { | |||
| // cast into the right context object | |||
| auto alignContext = std::any_cast<ContextType>(gctx); | |||
| auto alignContext = gctx.get<ContextType>(); | |||
asalzburger
pushed a commit
to asalzburger/acts
that referenced
this pull request
Feb 3, 2021
Move away from using a raw typedef to std::any. Doing that has certain drawbacks: std::any can be convert constructed from anything. This causes problems when constructing anything that only accepts a context any. GCC9 even has recursion trouble in the type_traits infrastructure leading to compile time failures.
Moving to wrappers can alleviate this. They only have an explicit constructor, meaning that they are not autoconstructed by conversion. This is a little bit more cumbersome to use, but much more robust I believe.
Performance should not be affected.
Previously, to break the problems of converting constructors with a single std::any argument, we resorted to std::reference_wrapper<std::any>, which only resolved it to some degree. This PR also removes this workaround. std::reference_wrapper<const {Geometry,MagneticField}Context> is still retained when used to store a context reference as a member, as for example in the stepper state.
BREAKING CHANGE: The public interface of the geometry and magnetic context type change. Before, they were raw std::anys, now they are of type Acts::ContextType. Construction needs to be explicit, and the contained std::any can be accessed via the Acts::ContextType::get() methods. std::any_cast to unpack the concrete context value is still supported, but needs to retrieve the internal std::any first. NOTE: If you forget to call .any() on the context object, std::any_cast will compile, but will throw std::bad_any_cast at runtime.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move away from using a raw typedef to std::any. Doing that has certain drawbacks: std::any can be convert constructed from anything. This causes problems when constructing anything that only accepts a context any. GCC9 even has recursion trouble in the type_traits infrastructure leading to compile time failures.
Moving to wrappers can alleviate this. They only have an explicit constructor, meaning that they are not autoconstructed by conversion. This is a little bit more cumbersome to use, but much more robust I believe.
Performance should not be affected.
Previously, to break the problems of converting constructors with a single
std::anyargument, we resorted tostd::reference_wrapper<std::any>, which only resolved it to some degree. This PR also removes this workaround.std::reference_wrapper<const {Geometry,MagneticField}Context>is still retained when used to store a context reference as a member, as for example in the stepper state.BREAKING CHANGE: The public interface of the geometry and magnetic context type change. Before, they were raw
std::anys, now they are of typeActs::ContextType. Construction needs to be explicit, and the containedstd::anycan be accessed via theActs::ContextType::get()methods.std::any_castto unpack the concrete context value is still supported, but needs to retrieve the internalstd::anyfirst. NOTE: If you forget to call.any()on the context object,std::any_castwill compile, but will throwstd::bad_any_castat runtime.