Implementation of clone() for all Workspaces #12951
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
SimonHeybrock
force-pushed
the
12687_workspace_clone_method
branch
from
e179294
to
910d35d
Compare
Added non-virtual clone() interface with a virtual private doClone() method to Workspace and derived classes. Workspaces that had a clone() method before were adapted to have a common interface everywhere. In cases where there was no clone() method previously we simply throw an error for now, until this new functionality is implemented. Some tests/mocks had to be adapted since there there is a pure virtual doClone() in all abstract classes. Do to smart pointer incompatibilities (mainly due to old boost versions) we had do extract the raw pointer from the unique_ptr returned by clone and put it into a boost::shared_ptr manually, in some cases. See also issue 12949.
This seems to be incompatible with the RHEL6 compiler.
Boost < 1.53 does not support construction from a std::unique_ptr, so for compatibility we must call clone().release() when dropping it to a boost::shared_ptr.
The previous code which derives from the old implementation of clone() did not call the base class copy constructor but the base class constructor. This seemed wrong (though I am not aware of any related issues).
Previously (before 12687) there was no implementation of clone(). That is, when calling clone() though a base class (e.g., ITableWorkspace) we would use TableWorkspace::clone() on an object of type MementoTableWorkspace. Was that doing the right thing?
Previously (before 12687) there was no implementation of clone(). That is, when calling clone() though a base class (e.g., ITableWorkspace) we would use TableWorkspace::clone() on an object of type SplittersWorkspace. Was that doing the right thing?
Note that this does not (yet?) support cloning a file-backed workspace (see CloneMDWorkspace algorithm).
SimonHeybrock
force-pushed
the
12687_workspace_clone_method
branch
from
e30c257
to
bfc4bae
Compare
Using the polymorpic clone, looking at the inheritance structure, code in CloneWorkspace could be unified. PeaksWorkspace is a child of ITableWorkspace, so Peaks* and Table* cast could be combined. EventWorkspace is a child of MAtrixWorkpace, so they could also be combined. The remaining special case is due to MDEventWorkspace, which still has some special code on CloneMDWorkspace.
SimonHeybrock
added
Framework
SimonHeybrock
force-pushed
the
12687_workspace_clone_method
branch
from
68e1d02
to
3427d5f
Compare
The reason for this change is related to the diamond inheritance that we had previously for SplittersWorkspace. VS2012 did not manage to compile this in combination with the virtual doClone() function with covariant return types. Just like IMaskWorkspace, ISplittersWorkspace is now not a Workspace anymore, and thus the diamond is gone.
SimonHeybrock
force-pushed
the
12687_workspace_clone_method
branch
from
b3ea139
to
a941ef3
Compare
|
This looks good now that the MSVC issues are sorted. |
martyngigg
added a commit
that referenced
this pull request
Jul 3, 2015
…thod Implementation of clone() for all Workspaces
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.
Closes #12687.
Implements
Workspace::clone()with covariant return type in all sub-classes. Note that there is one remaining issue when cloning a file-backed MDEventWorkspace, see #12994.The copy implementation by
CloneWorkspacewas generally replaced by the newclone()method. Therefore the tests ofCloneWorkspacetest the changes in these commits. Furthermore a few tests of theclone()method itself were added, but the actual coverage is probably by far not complete.