search

libMesh / libmesh

libMesh github repository
http://libmesh.github.io
GNU Lesser General Public License v2.1
654 stars 287 forks source link

Why not more mesh copy construction? #2079

Open lindsayad opened 5 years ago

lindsayad commented 5 years ago

Snippet of MeshBase copy constructor:

MeshBase::MeshBase (const MeshBase & other_mesh) :
  ParallelObject (other_mesh),
  boundary_info  (new BoundaryInfo(*this)),
  _n_parts       (other_mesh._n_parts),
  _is_prepared   (other_mesh._is_prepared),
  _point_locator (),
  _count_lower_dim_elems_in_point_locator(other_mesh._count_lower_dim_elems_in_point_locator),
  _partitioner   (),
#ifdef LIBMESH_ENABLE_UNIQUE_ID
  _next_unique_id(other_mesh._next_unique_id),
#endif
  _skip_noncritical_partitioning(false),
  _skip_all_partitioning(libMesh::on_command_line("--skip-partitioning")),
  _skip_renumber_nodes_and_elements(false),
  _allow_remote_element_removal(true),
  _elem_dims(other_mesh._elem_dims),
  _spatial_dimension(other_mesh._spatial_dimension),
  _default_ghosting(new GhostPointNeighbors(*this)),
  _ghosting_functors(other_mesh._ghosting_functors),
  _point_locator_close_to_point_tol(0.)

What is the reason that we don't copy many of the data member of other_mesh? I'm currently particularly interested in the allow_remote_element_removal flag but the general question stands. It might be nice to document reasons for why we don't copy construct some of these members because I'm sure there are good reasons for many/all of them that might be very instructive regarding the mesh construction process.

roystgnr commented 5 years ago

What is the reason that we don't copy many of the data member of other_mesh?

...

Answering the question about proximate reasons: I'm embarrassed to say that I don't remember. Looking through the git log suggests that those entries are 2.5y old or more, which reduces my embarrassment slightly.

_default_ghosting is a fixed thing (you change its effects by pulling it out of the ghosting functors list not by actually changing it) stored by unique_ptr to avoid sucking in more header dependencies, so a new initialization is equivalent to a deep copy there.

The _skip_noncritical_partitioning behavior was aping the previous _skip_partitioning behavior, and probably ought to be changed. Likewise for the previous (now _skip_all_partitioning) behavior; it looks like I just changed that thoughtlessly when we added the command line option.

_point_locator_close_to_point_tol came from @jwpeterson so I'll let him comment.

_allow_remote_element_removal was me, and probably ought to be changed.

_point_locator gets generated when needed so it's probably cheaper to keep doing that than to preemptively copy an existing one; when a Mesh is copied that's often with the intention of mutating the copy before use.

_partitioner gets cloned if applicable in the copy constructor body

_skip_renumber_nodes_and_elements was defaulted to false by @friedmud - but that was in 2012 so he may not remember why. ;-)

...

Answering the question one distal level back: originally IIRC a Mesh was a heavyweight object that couldn't even be copied, so even when that was fixed people hadn't gotten into the habit of doing interesting things by copying meshes, so the series of dubious design choices above never got properly scrutinized. If you're thinking about adding PRs to make our copy constructors more properly copy constructors, pragmatically I can't guarantee there won't be side effects for us to work out but philosophically I'd approve of the changes.

jwpeterson commented 5 years ago

_point_locator_close_to_point_tol came from @jwpeterson so I'll let him comment.

This was just an oversight.

It's very easy to forget to update copy constructors when adding new data members to classes... an overall annoying thing about C++ IMO. Anything not copied just gets "default" initialized, which for POD types just means uninitialized, which is really bad! I wonder if there's a way to get compiler warnings when you do this...