RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.17k stars 1.24k forks source link

[geometry] BVH structs lack explicit move/copy semantics #14073

Closed SeanCurtis-TRI closed 3 months ago

SeanCurtis-TRI commented 3 years ago

From a discussion that came up in #13978.

Summary

We've exercised DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(BvNode). We have no tests supporting that declaration and it has members with types drawn from a) a nested class and b) an external struct which don't have explicit move and copy semantics addressed. This is counter to the Drake style guide:

Every class's public interface should make explicit which copy and move operations the class supports.

Comment verbatim

The following text is copied-and-pasted from the discussion link provided above as a convenience.


An excellent point. After considering, I'm going to say the following:

  1. BvNode has explicitly declared DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(BvNode).
  2. In order for that declaration to be valid, all of its members must have full copy and move semantics. A reader of the code should have some assurance that there is, at least, the clear intent that those members have the same semantics. It's worth noting, if it has a child member that doesn't have those semantics, it will still compile as long as we don't use it in a context that requires those semantics. So, the declaration without tested support is a land mine.
  3. Its members includes LeafData and nested struct NodeChildren.
  4. DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN applies to struct as well as class.
  5. Therefore, we should make sure both LeafData and NodeChildren obviously have the required semantics so future readers don't have to reason too hard about it.

For LeafData, simply add DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(LeafData) and for NodeChildren, because of the existence of the copy constructor, we'll need to finish accounting for all of the other copy semantics.

jwnimmer-tri commented 3 months ago

Next time we have code in PR where this is related, we'll ding it and fix it then. We don't need a tracking issue for >4 years -- empirically it's not really urgent.