Should I convert shared_ptr to weak_ptr when passed to a method?

Question

there are already a couple of questions regarding this topic, but I am still not sure what to do: Our codebase uses shared_ptr at many places. I have to admit that we did not define ownership clearly when writing it.

We have some methods like

 void doSomething(shared_ptr<MyClass> ptr)
 {
      //doSomething() is a member function of a class, but usually won't store the ptr
      ptr->foo();
      ...
 }

After having discovered the first (indirect) circular dependencies I would like to correct the mistakes in our design. But I'm not exactly sure how. Is there any benefit in changing the method from above to

 void doSomething(weak_ptr<MyClass> ptr)
 {
      shared_ptr<MyClass> ptrShared = ptr.lock();
      ptrShared->foo();
      ...
 }

?

I am also confused because some people say (including the Google Style guide) that in the first place it's important to get ownership correct (which would probably mean introduction of many weak_ptrs, e.g. in the example with the methods above, but also for many member variables that we have). Others say (see links below) that you should use weak_ptr to break cyclic dependencies. However, detecting them is not always easy, so I wonder if I really should use shared_ptr until I run into problems (and realize them), and then fix them??

Thanks for your thoughts!

See also

• shared_ptr and weak_ptr differences
• boost::shared_ptr cycle break with weak_ptr
• boost, shared ptr Vs weak ptr? Which to use when?

Answer 1

We did not define ownership clearly.

You need to clearly define who owns what. There's no other way to solve this. Arbitrarily swapping out some uses of shared_ptr with weak_ptr won't make things better.

Answer 2

There is no benefit in changing your design above from shared_ptr to weak_ptr. Getting ownership right is not about using weak_ptrs, it's about managing who stores the shared_ptr for any significant length of time. If I pass a shared_ptr to a method, assuming I don't store that shared_ptr into a field in my object as part of that method, I haven't changed who owns that data.

In my experience the only reason for using weak_ptr is when you absolutely must have a cycle of pointers and you need to break that cycle. But first you should consider if you can modify your design to eliminate the cycle.

I usually discourage mixing shared_ptr's and raw pointers. It inevitably happens (though it probably shouldn't) that a raw pointer needs to be passed to a function that takes a shared_ptr of that type. A weak_ptr can be safely converted to a shared_ptr, with a raw pointer you're out of luck. Even worse, a developer inexperienced with shared_ptr's may create a new shared_ptr from that raw pointer and pass it to the function, causing that pointer to be deleted when the function returns. (I actually had to fix this bug in production code, so yes it does happen :) )