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

feat: Add polymorphic value like container #855

Merged
merged 15 commits into from
Jun 25, 2021

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Jun 24, 2021

This PR introduces a type PolymorphicValue<T> that behaves very similar to std::unique_ptr<T>, but preserves knowlegde of how to copy the value. In fact, PolymorphicValue only works with copyable types.

This allows more convenient handling of interfaces, for types that are copyable and where copying does not have to be avoided.
It looks like this:

struct Base {};
struct Derived {};

std::unique_ptr<Base> up1 = std::make_unique<Derived>();
std::unique_ptr<Base> up2 = up1; // this does not work

Acts::PolymorphicValue<Base> pv1 = Acts::makePolymorphicValue<Derived>();
Acts::PolymorphicValue<Base> pv2 = pv1; // this makes a copy of the stored object

The reason why I'm adding this is that pybind11 does not support std::unique_ptr as arguments/values on the C++ side, since python lacks a mechanism to release ownership, which would be required in this case. There's a limited number of cases where this is a problem, and where we don't actually care if we copy the stored value.

WIP until I finalize this.

@paulgessinger paulgessinger added the 🚧 WIP Work-in-progress label Jun 24, 2021
@paulgessinger paulgessinger added this to the next milestone Jun 24, 2021
@codecov
Copy link

codecov bot commented Jun 24, 2021

Codecov Report

Merging #855 (da904dd) into main (c7ad7e8) will increase coverage by 0.12%.
The diff coverage is 68.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #855      +/-   ##
==========================================
+ Coverage   48.53%   48.65%   +0.12%     
==========================================
  Files         329      330       +1     
  Lines       16974    17076     +102     
  Branches     8025     8053      +28     
==========================================
+ Hits         8239     8309      +70     
- Misses       3083     3089       +6     
- Partials     5652     5678      +26     
Impacted Files Coverage Δ
Core/include/Acts/Utilities/PolymorphicValue.hpp 68.62% <68.62%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7ad7e8...da904dd. Read the comment docs.

@paulgessinger
Copy link
Member Author

@HadrienG2 would you be available to review?

@paulgessinger paulgessinger removed the 🚧 WIP Work-in-progress label Jun 25, 2021
Copy link
Contributor

@HadrienG2 HadrienG2 left a comment

Choose a reason for hiding this comment

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

Makes sense.

@paulgessinger paulgessinger merged commit a837633 into acts-project:main Jun 25, 2021
@paulgessinger paulgessinger deleted the polymorphic-value branch June 25, 2021 15:00
@gagnonlg
Copy link
Contributor

@paulgessinger how come the CI didn't try a build for this PR? It broke the build due to a missing #include <optional> in PolymorphicValueTest.cpp.

paulgessinger pushed a commit that referenced this pull request Jun 27, 2021
#855 broke the build on my end because of a missing include in a test file. Here we simply add that include, and all is well.
@paulgessinger paulgessinger modified the milestones: next, v9.2.0 Jul 8, 2021
asalzburger pushed a commit that referenced this pull request Aug 24, 2021
This is needed to ensure G4 can have ownership of the detector constructions.
This also makes the recently introduced polymorphic value container (#855) obsolete, so
this removes it
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.

None yet

3 participants