Project

General

Profile

Bug #12067

Add move operations to simb::MCParticle

Added by Gianluca Petrillo over 3 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Start date:
03/25/2016
Due date:
% Done:

100%

Estimated time:
Spent time:
Duration:

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

Blocks LArSoft - Necessary Maintenance #18199: Remove unmoveable MCParticle workaroundClosed11/10/2017

Blocked by lar_ci - Feature #18378: Add a LArSoft-based workflow pulling in nusimdata and nutools sourcesClosed11/27/2017

Associated revisions

Revision b4016939 (diff)
Added by Gianluca Petrillo about 2 years ago

Added move constructor and assignment to simb::MCParticle

This solves issue #12067.

Revision 8f959c05 (diff)
Added by Gianluca Petrillo about 2 years ago

Removed virtual destructor from simb::MCParticle

This implements the additional suggestion from issue #12067.

Revision 99756591 (diff)
Added by Lynn Garren over 1 year ago

v1_12_00 resolves #12067

Revision e50ed83c
Added by Lynn Garren over 1 year ago

Merge tag 'v1_12_00' into develop

v1_12_00 resolves #12067 v1_12_00

Revision d853761c (diff)
Added by Lynn Garren over 1 year ago

v2_21_00 resolves #12067

Revision 9ec4fd8b (diff)
Added by Lynn Garren over 1 year ago

v2_21_00 resolves #12067

Revision 0a09b246 (diff)
Added by Lynn Garren over 1 year ago

v2_21_00 resolves #12067

Revision d853761c (diff)
Added by Lynn Garren over 1 year ago

v2_21_00 resolves #12067

Revision 537e8b0d (diff)
Added by Lynn Garren over 1 year ago

v2_21_00 resolves #12067

Revision d853761c (diff)
Added by Lynn Garren over 1 year ago

v2_21_00 resolves #12067

Revision e8661061
Added by Lynn Garren over 1 year ago

Merge tag 'v2_21_00' into develop

v2_21_00 resolves #12067 v2_21_00

Revision 8b4bc7df
Added by Lynn Garren over 1 year ago

Merge tag 'v2_21_00' into develop

v2_21_00 resolves #12067 v2_21_00

Revision 0fc94d04
Added by Lynn Garren over 1 year ago

Merge tag 'v2_21_00' into develop

v2_21_00 resolves #12067 v2_21_00

Revision e8661061
Added by Lynn Garren over 1 year ago

Merge tag 'v2_21_00' into develop

v2_21_00 resolves #12067 v2_21_00

Revision 6b9de9c9
Added by Lynn Garren over 1 year ago

Merge tag 'v2_21_00' into develop

v2_21_00 resolves #12067 v2_21_00

Revision e8661061
Added by Lynn Garren over 1 year ago

Merge tag 'v2_21_00' into develop

v2_21_00 resolves #12067 v2_21_00

History

#1 Updated by Gianluca Petrillo over 3 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 about 2 years ago

#3 Updated by Lynn Garren about 2 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 about 2 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 about 2 years ago

  • Status changed from Feedback to Assigned
  • Assignee changed from Brian Rebel to Gianluca Petrillo

#6 Updated by Gianluca Petrillo about 2 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 about 2 years ago

Brian Rebel has given the all-clear to the inclusion of the changes into nusimdata.

#8 Updated by Gianluca Petrillo about 2 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 about 2 years ago

  • Blocked by Feature #18378: Add a LArSoft-based workflow pulling in nusimdata and nutools sources added

#10 Updated by Gianluca Petrillo about 2 years ago

  • Priority changed from High to Normal

#11 Updated by Lynn Garren over 1 year ago

  • Status changed from Work in progress to Resolved
  • % Done changed from 90 to 100

#12 Updated by Gianluca Petrillo over 1 year ago

  • Status changed from Resolved to Closed


Also available in: Atom PDF