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

MeshBase move constructor can't be safely defaulted #2732

Merged
merged 2 commits into from
Oct 6, 2020

Conversation

lindsayad
Copy link
Member

@lindsayad lindsayad commented Oct 5, 2020

This is because of the way we construct the BoundaryInfo member. Using
the default move constructor, our BoundaryInfo in the move-constructed
object will have an invalid reference to its _mesh member after the other_mesh
is destructed.

@jwpeterson
Copy link
Member

Hmm yeah I see the problem, but I hate having yet another copy/paste maintenance issue in mesh_base.C to go along with the copy constructor. Is there any way that we can just delete the move constructor for now? I suppose if we found that we were spending a lot of time copying meshes we could look into implementing it...

@jwpeterson
Copy link
Member

Though the issues with #2710 means that we won't be able to store Mesh objects directly in std::vectors without this... if that is important it could also warrant a non-default implementation of course but IMO holding vectors of unique_ptrs is usually going to be a good workaround for non-MoveInsertable objects.

@lindsayad
Copy link
Member Author

Ugh I suppose I can just continue to operate with unique_ptr instead of instances. I will just delete the move constructor as recommended

@jwpeterson
Copy link
Member

Well I'm OK either way. If you promise to maintain the hand-coded MeshBase move ctor :)

@roystgnr
Copy link
Member

roystgnr commented Oct 5, 2020

I also vote for @lindsayad to be in charge of maintaining the move constructor.

@lindsayad
Copy link
Member Author

If I come up with a convincing reason to have a move constructor beyond programming laziness (which would actually incur real run-time expense since moving a unique_ptr is trivial), then I will add the implementation back

Copy link
Member

@jwpeterson jwpeterson left a comment

Choose a reason for hiding this comment

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

This definitely fixes potential bugs, so seems like it should be cherry-picked onto 1.6.0.

This is because of the way we construct the `BoundaryInfo` member. Using
the default move constructor, our `BoundaryInfo` in the move-constructed
object will have an invalid reference to its `_mesh` member after the
move constructor method is finished
@lindsayad
Copy link
Member Author

lindsayad commented Oct 5, 2020

merge conflict sad. Oooo I keep the approval though! I like that

@lindsayad lindsayad merged commit a400d51 into libMesh:master Oct 6, 2020
jwpeterson pushed a commit that referenced this pull request Oct 6, 2020
The MeshBase move constructor can't be safely defaulted because of the
way we construct the `BoundaryInfo` member. Using the default move
constructor, our `BoundaryInfo` in the move-constructed object will
have an invalid reference to its `_mesh` member after the move
constructor method is finished

Also remove the Mesh move constructor unit test that we had previously.

This is a combination of the following two commits cherry-picked from
master:

4a48915
681e9ad

Refs #2732
@lindsayad lindsayad deleted the mesh-base-move-constructor branch October 8, 2020 20:31
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.

3 participants