Drake: Use of std::shared_ptr is likely excessive

[david-german-tri]

The style guide discourages shared ownership, but std::shared_ptr is ubiquitous in Drake. We should work out the actual ownership semantics and determine whether some or all of these uses can be replaced. In general, we should be able to replace shared_ptr with value semantics or unique_ptr, using move semantics to transfer ownership, and const references or raw pointers to grant non-ownership access.

@amcastro-tri, I know this topic is also of interest to you.

[sherm1]

I agree! I'd like us to consider const& the default type for arguments, with anything else at least raising some eyebrows in review (especially raw pointers!). A related item is that simple stack allocated objects should be the default -- I see a lot of std::make_shared<T> allocations where I wonder if just plain T would have worked.

[rpoyner-tri]

Every call to make_shared should cost the author a dollar? ;)

[sherm1]

Every call to make_shared should cost the author a dollar?

:+1: Or for each make_shared you have to pick one file and make it Styleguide-compliant!

[david-german-tri]

@sherm1 I actually think raw pointers are the best way to let a callee mutate an object without transferring ownership. It's in the style guide and policed by cpplint. Otherwise, I agree, especially about preferring value semantics where feasible.

[sherm1]

I actually think raw pointers are the best way to let a callee mutate an object without transferring ownership.

Oh, that's interesting -- thanks David. I hadn't seen that convention. Is it also required that the called method won't attempt to take ownership or keep a copy of the pointer?

[sammy-tri]

https://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers indicates that it's preferred to use smart pointers (unique_ptr or shared_ptr) to transfer or share ownership.

[david-german-tri]

Is it also required that the called method won't attempt to take ownership

Yes.

or keep a copy of the pointer?

Not necessarily. For instance, a ClassThatOwnsPointer might want to construct a HelperClassThatUsesPointerExtensively, in which case the helper class could take the pointer in its constructor. The function that hangs onto a copy of a pointer should simply be commented to indicate how long the pointer must remain valid, e.g. the lifetime of the helper class. Same for a function that hangs onto a const &.

[david-german-tri]

@sammy-tri Yes. access != ownership. (We might be agreeing. I'm not sure to whom you were responding.)

[sammy-tri]

I think we're in agreement. :)

[patmarion]

This thread seems to be biased against reference counted pointers, so I'll chime in :) I would vote against using unique_ptr in the public api. I think it makes code awkward and restrictive.

But more than anything else, my favorite apis are those that are consistent. When api mixes raw pointers, shared pointers, unique pointers, values and references, I find it makes writing code for that library very difficult. You'll find user code doing all sorts of odd pointer conversions just to get their arguments into the right format to call your function. The code should all work together to be natural and seamless.

My suggestion is to use what's best for the type, and then be consistent. For some types, pass by value is the most sensible, but others, it makes more sense to use reference counted pointers. So I think that a blanket statement like "shared_ptr is bad" is just not fair or true.

[david-german-tri]

@patmarion In code that follows the style guide, there aren't any "odd conversions". There's a clear hierarchy of pointer types with conversion in the downward direction only:

> value or unique_ptr: mutable, owned
> > raw pointer: mutable, unowned
> > > const reference: immutable, unowned

[david-german-tri]

Kudos to @amcastro-tri for some major progress on shared_ptr cleanup! I don't think this issue is a useful way to track or motivate progress anymore, so I'm going to close it, but feel free to keep citing it in PRs and so on.

[amcastro-tri]

It still bothers me that we are forced to used shared_ptr's when instantiating a BotVisualizer. Also cascade takes shared_ptrs!!! I guess this is something that will get cleaned up with System 2.0?

[david-german-tri]

Yes, that definitely goes away in System2.