Ticket #5862 (closed: fixed)
Suspicious copy constructor in MDEvent workspace. Possible Defect
| Reported by: | Alex Buts | Owned by: | Owen Arnold |
|---|---|---|---|
| Priority: | major | Milestone: | Release 2.4 |
| Component: | Mantid | Keywords: | |
| Cc: | Blocked By: | ||
| Blocking: | Tester: | Russell Taylor |
Description (last modified by Alex Buts) (diff)
Unlike other workspaces, MDEvent workspace has a copy constructor (the deep one).
Leaving aside question if the workspace needs one, the copy constructor behaviour looks strange.
Shared pointer to a Box controller which belongs to a workspace and used when splitting boxes is usually referred in each box, the controller used to split.
when one deploys a copy constructor, new deep copy of Box controlled is created in the new workspace. In addition to that, new deep copies of boxes with data are created in the new workspace. But these boxes contain the copy of shared pointer to the old Box controller. This would lead to very strange behaviour, which is difficult to predict.
At least, it looks like that to me. I am not sure if this is bug, feature or may be I just missing something.
But it worth putting this ticket for noting in a future.
Change History
comment:2 Changed 8 years ago by Nick Draper
- Milestone changed from Release 2.3 to Release 2.4
Moved to milestone 2.4
comment:3 Changed 8 years ago by Nick Draper
- Owner set to Owen Arnold
- Status changed from new to assigned
Not completely sure what to do about this one.
Owen could I get you opinion on this?
comment:4 Changed 8 years ago by Owen Arnold
refs #5862. Swap boxcontroller as part of mdew copy.
Changeset: 6dc34c0071575c8a77a06c7038da5313177707bd
comment:5 Changed 8 years ago by Owen Arnold
refs #5862. Call base method properly.
Changeset: 62e7bf24f60fd26f65fb6e1982f1498cfb4ed0f3
comment:6 Changed 8 years ago by Owen Arnold
refs #5862. Fix test macro usage.
Changeset: 99e1ef96c541215cfef30caa15497fc5faf06f87
comment:8 Changed 8 years ago by Owen Arnold
- Status changed from accepted to verify
- Resolution set to fixed
Tester:
I started work on this defect by adding regression test/assertions to the copy constructor test in MDEventWorkspaceTest. These failed, because the reference to the new box controller was never set on the copied MDBoxBases. I then set about fixing this regression test.
There is no easy way to test this other than by looking at the unit tests. I suggest code reviewing the changes and the unit tests added.
comment:9 Changed 8 years ago by Owen Arnold
refs #5862. Swap boxcontroller as part of mdew copy.
Changeset: 6dc34c0071575c8a77a06c7038da5313177707bd
comment:10 Changed 8 years ago by Owen Arnold
refs #5862. Call base method properly.
Changeset: 62e7bf24f60fd26f65fb6e1982f1498cfb4ed0f3
comment:11 Changed 8 years ago by Owen Arnold
refs #5862. Fix test macro usage.
Changeset: 99e1ef96c541215cfef30caa15497fc5faf06f87
comment:12 Changed 8 years ago by Russell Taylor
- Status changed from verify to verifying
- Tester set to Russell Taylor
comment:13 Changed 8 years ago by Russell Taylor
- Status changed from verifying to closed
- Summary changed from Suspishious copy constructor in MDEvent workspace. Possible Defect to Suspicious copy constructor in MDEvent workspace. Possible Defect
Tested by inspection.
(Although it wasn't something changed under this ticket, I did find myself wondering why MDBox(Base) copy didn't make use of a virtual copy constructor (e.g. clone).)
comment:14 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 6708
