Bug #12067
Add move operations to simb::MCParticle
Description
The class simb::MCParticle is written in a way that explicitly prevents move operations.
In fact, move operations are implemented with copy, that does not free the memory in excess.
For example, if my particle has 5 trajectory points and I move into it another particle with 3 trajectory points, my particle will still have memory allocated for 5 points.
The solution is simply to explicitly add default move constructors in SimulationBase/MCTrajectory.h:
MCParticle(MCParticle&&) = default;
MCParticle& operator= (MCParticle&&) = default;
I am elevating the priority of this because it's a straightforward change that may have immediate, transparent gain.
Related issues
Associated revisions
Removed virtual destructor from simb::MCParticle
This implements the additional suggestion from issue #12067.
v1_12_00 resolves #12067
Merge tag 'v1_12_00' into develop
v1_12_00 resolves #12067 v1_12_00
v2_21_00 resolves #12067
v2_21_00 resolves #12067
v2_21_00 resolves #12067
v2_21_00 resolves #12067
v2_21_00 resolves #12067
v2_21_00 resolves #12067
Merge tag 'v2_21_00' into develop
v2_21_00 resolves #12067 v2_21_00
Merge tag 'v2_21_00' into develop
v2_21_00 resolves #12067 v2_21_00
Merge tag 'v2_21_00' into develop
v2_21_00 resolves #12067 v2_21_00
Merge tag 'v2_21_00' into develop
v2_21_00 resolves #12067 v2_21_00
Merge tag 'v2_21_00' into develop
v2_21_00 resolves #12067 v2_21_00
Merge tag 'v2_21_00' into develop
v2_21_00 resolves #12067 v2_21_00
History
#1 Updated by Gianluca Petrillo over 4 years ago
While doing this, it might be worth also removing the implementation of the destructor (that is empty) and declare it as
virtual ~MCParticle() = default;
#2 Updated by Gianluca Petrillo almost 3 years ago
- Blocks Necessary Maintenance #18199: Remove unmoveable MCParticle workaround added
#3 Updated by Lynn Garren almost 3 years ago
- Status changed from New to Feedback
- Assignee set to Brian Rebel
Brian, are you able to do this? If not, is it OK if Gianluca does this?
#4 Updated by Brian Rebel almost 3 years ago
Lynn Garren wrote:
Brian, are you able to do this? If not, is it OK if Gianluca does this?
I have no objections to Gianluca doing it.
#5 Updated by Lynn Garren almost 3 years ago
- Status changed from Feedback to Assigned
- Assignee changed from Brian Rebel to Gianluca Petrillo
#6 Updated by Gianluca Petrillo almost 3 years ago
- Status changed from Assigned to Work in progress
- % Done changed from 0 to 90
The two suggested changes are implemented in branch feature/gp_Issue12067 of nusimdata.
The other stakeholders of nutools should confirm that the removal of the virtual table from simb::MCParticle has no consequence on their code.
The only issues I can think of are when using dynamic_cast<simb::MCParticle>(...) or typeid(simb::MCParticle).
As a reminder, art discourages data product classes from being polymorphic.
#7 Updated by Gianluca Petrillo almost 3 years ago
Brian Rebel has given the all-clear to the inclusion of the changes into nusimdata.
#8 Updated by Gianluca Petrillo almost 3 years ago
For the most complete testing, the integration tests should be run with the experiment code.
Feature request #18378 has been submitted and accepted. If the feature therein is implemented in the short term, I'll use it to perform said test.
#9 Updated by Gianluca Petrillo almost 3 years ago
- Blocked by Feature #18378: Add a LArSoft-based workflow pulling in nusimdata and nutools sources added
#10 Updated by Gianluca Petrillo almost 3 years ago
- Priority changed from High to Normal
#11 Updated by Lynn Garren over 2 years ago
- Status changed from Work in progress to Resolved
- % Done changed from 90 to 100
Applied in changeset nusimdata:997565914af233bec48acc832fada59bc4f21ecd.
#12 Updated by Gianluca Petrillo over 2 years ago
- Status changed from Resolved to Closed
Added move constructor and assignment to simb::MCParticle
This solves issue #12067.