What is the correct use of shared pointers?

[furgalep]

We use shared pointers for memory management but I still have some questions on what is the best use.

Let's say I have a class, Frame, that has a shared_ptr member Camera. The camera is shared among many frames. I have some questions about best practices:

class Frame {
public:
  // Q1
  Frame(std::shared_ptr<Camera> camera) : camera_(camera) {}

  // Q2
  std::shared_ptr<Camera> getCamera() const{ return camera_; }

  // Q3
  void setCamera( std::shared_ptr<Camera> camera ) { camera_ = camera; }
private:
  std::shared_ptr<Camera> camera_;
};

Questions:

Q1. What should the constructor take as an argument: std::shared_ptr<Camera>, or const std::shared_ptr<Camera>&, or something else?

Q2. What should getCamera() return, std::shared_ptr<Camera>, or const std::shared_ptr<Camera>&, or const std::shared_ptr<const Camera>&, or something else? Or should we have a const and a non-cost version. And should we name the non-const version getCameraMutable()?

Q3. What should setCamera() take as an argument: std::shared_ptr<Camera>, or const std::shared_ptr<Camera>&, or something else?

[furgalep]

@simonlynen, @schneith, anyone else?

[uschwes]

Isn't the reference counter screwed up when you pass shared pointer by reference?

[PascalGohl]

I am following this suggestion: "Shortly, there is no reason to pass by value, unless there is multi-threading going on, but this needs to be considered separately." From: http://stackoverflow.com/a/8741626

[uschwes]

Oh, I see. Ok when you assign to member directly.

Shouldn't there be also a method to make the refered object const?

std::shared_ptr<const Camera> getCamera() const;

How would that method look like if the member is a ´´´std::shared_ptr´´´?

[HannesSommer]

About Q2:

• I don't think one can fully answer this in general as it depends very much on the whole design concerning the (relative) lifetimes of Camera and Frame object and the access rights to them. And as a consequence there can't be a catch all best practice for that.
• Usecase where this method could be non const (as a member function) seem quite unlikely! That would mean somebody could change what the pointer points to from outside and would therefor need to return a nonconst reference to the shared pointer. If that is really necessary it would certainly be a mutable getter!
• All other cases don't seem to be "mutable" cases as it is in such cases not a member you give access to but a shared object. This way it is not a classical getter at all. Why not getSharedCamera() or getCameraShared()?
• Are you sure you want/need to reveal the shared nature in such a getter, giving access to the camera? You could just return a const ref to the camera. Returning a shared ptr only makes sense if there is a usecase where a user wants or needs to share ownership. When you return reference people should not expect this reference to live longer than the object they acquired it from. As the shared pointer is a member of Frame this is most likely already guarantied here, unless you allow write access to that pointer member. You cannot prevent users from storing pointers to the camera just by returning it as a shared_ptr anyways if that is the idea of returning a shared_ptr. And saving a pointer / reference retrieved by a const ref getter always requires thinking about the lifetime of the returned object.
• Whether to return a shard_ptr<const Camera> is a totally different question. It is much more a question of the whole design. If that const was necessary (it won't be optional) you would know by the design part concerning how to get write access to a camera. Either it should be possible via a Frame or not. If you decide that it is clear which kind of getters you can have here.

[HannesSommer]

Again Q2: To be fast I would return const std::shared_ptr<Camera>& rather than std::shared_ptr<Camera>, and ideally make the getter inline (= don't prevent it by defining it in a cpp file), to have the optimizers optimize the reference to an pointer indirection. Why would one want that extra copy into a temporary object (yielding ownership management overhead)? Move semantics or return value optimization is impossible here, as you return a member, which isn't a temporary object. Of course it that temporary is assigned to a local it will be moved but that is still less efficient than directly copying from a reference. And in some cases it won't be assigned, like e.g. in things like: frame->getCamera()->getStuff(). Of course multi threading can again change a lot here...