[geometry] Add GeometryInstance overload without unique_ptr

[jwnimmer-tri]

Towards #19250.

Broadly, a C++ function that takes a unique_ptr<T> as input without also offering a const T& overload is defective in that it forces users (even C++ users) to worry about lifetimes and leaves our implementations vulnerable to unwanted aliasing, when in most cases they don't care about actually care about ownership transfer. See, e.g., #14453 for prior art.

---

This change is 

[jwnimmer-tri]

+@SeanCurtis-TRI for feature review, please.

[SeanCurtis-TRI]

With one quesiton.

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

---

geometry/geometry_instance.h line 95 at r1 (raw file):

  /** (Advanced) Overload that transfers ownership of `shape` (for performance).
   The caller must not retain any pointer to `shape`.

BTW This comment seems largely inspired by thinking about python users. It strikes me as being either redundant or misleading based on the function signature. Can you explain the motivation behind this additional comment?

Code quote:

The caller must not retain any pointer to `shape`.

[SeanCurtis-TRI]

+@ggould-tri for platform review, please.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri and @jwnimmer-tri)

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri and @SeanCurtis-TRI)

---

geometry/geometry_instance.h line 95 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This comment seems largely inspired by thinking about python users. It strikes me as being either redundant or misleading based on the function signature. Can you explain the motivation behind this additional comment?

The caller is transferring the lifetime of shape to be under the control of GeometryInstance (and in all probability, eventually the SceneGraph). Those classes might mutate the Shape, or copy it, or delete it, or otherwise alter it in ways that the caller cannot anticipate, e.g., by calling release_shape. The idea of this comment is to dissuade callers (even C++ callers) from trying to keep a borrowed pointer around. We don't want to promise that the borrowed pointer will remain valid.

[ggould-tri]

(I'd modify your manifesto statement above to limit it to cases where T is copyable/cloneable -- for non-copyable T it is a more complicated case. Of course we know we're in the copyable case here (per the copyable_unique_ptr), so your argument strikes with full force.)

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @SeanCurtis-TRI)

[SeanCurtis-TRI]

Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

---

geometry/geometry_instance.h line 95 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The caller is transferring the lifetime of shape to be under the control of GeometryInstance (and in all probability, eventually the SceneGraph). Those classes might mutate the Shape, or copy it, or delete it, or otherwise alter it in ways that the caller cannot anticipate, e.g., by calling release_shape. The idea of this comment is to dissuade callers (even C++ callers) from trying to keep a borrowed pointer around. We don't want to promise that the borrowed pointer will remain valid.

I guess where I'm being obtuse is how is that not equally true of every API that takes a unique_ptr by value? That felt intrinsic to the concept of passing ownership.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

---

geometry/geometry_instance.h line 95 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I guess where I'm being obtuse is how is that not equally true of every API that takes a unique_ptr by value? That felt intrinsic to the concept of passing ownership.

Ah. So you're saying comment is redundant, not that it's wrong?

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

---

geometry/geometry_instance.h line 95 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ah. So you're saying comment is redundant, not that it's wrong?

My original comment is that it could be "either redundant or misleading". I believe we can both see the arguable redundancy.

By "misleading" I was referencing the idea that passing ownership doesn't ever preclude the ability to hang on to a raw pointer. But that behavior has always been dangerous, because you can't know what happens to the object after passing ownership. But we've rarely warned people about whether or not the thing that gets passed is disassembled (and destroyed) or not. In the case of the Shape, it isn't and the shape stays alive indefinitely Certainly, if you store a pointer to data that some other code owns, there is always the danger of it mutating, unless you've been given a clear promise to the contrary.

So, you can always hang on to a raw pointer with just as much safety in this API as every other similar API.

(In the case of the GeometryInstance passed to SceneGraph, it is decomposed. But that's a different API.)

[jwnimmer-tri]

Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

---

geometry/geometry_instance.h line 95 at r1 (raw file):

In the case of the Shape, it isn't and the shape stays alive indefinitely.

I want to leave room to change this implementation behavior down the road, e.g., immediately Clone the argument and discard the the original. From the point of view of the caller, they should assume no possibility of the shape pointer remaining valid upon return. That's what I'm trying to communicate here.

If we think that's sufficiently obvious to a regular C++ users, we could nix the extra sentence.

[SeanCurtis-TRI]

Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

---

geometry/geometry_instance.h line 95 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

In the case of the Shape, it isn't and the shape stays alive indefinitely.

I want to leave room to change this implementation behavior down the road, e.g., immediately Clone the argument and discard the the original. From the point of view of the caller, they should assume no possibility of the shape pointer remaining valid upon return. That's what I'm trying to communicate here.

If we think that's sufficiently obvious to a regular C++ users, we could nix the extra sentence.

Yep. I think the idea of unique_ptr means as soon as you've passed it along, you have no guarantees. So, I'd be quite happy with removing the extra sentence.

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)