C++ - passing references to boost::shared_ptr

Question

If I have a function that needs to work with a shared_ptr, wouldn't it be more efficient to pass it a reference to it (so to avoid copying the shared_ptr object)? What are the possible bad side effects? I envision two possible cases:

1) inside the function a copy is made of the argument, like in

ClassA::take_copy_of_sp(boost::shared_ptr<foo> &sp)  
{  
     ...  
     m_sp_member=sp; //This will copy the object, incrementing refcount  
     ...  
}

2) inside the function the argument is only used, like in

Class::only_work_with_sp(boost::shared_ptr<foo> &sp) //Again, no copy here  
{    
    ...  
    sp->do_something();  
    ...  
}

I can't see in both cases a good reason to pass the boost::shared_ptr by value instead of by reference. Passing by value would only "temporarily" increment the reference count due to the copying, and then decrement it when exiting the function scope. Am I overlooking something?

Andrea.

EDIT:
Just to clarify, after reading several answers : I perfectly agree on the premature-optimization concerns, and I alwasy try to first-profile-then-work-on-the-hotspots. My question was more from a purely technical code-point-of-view, if you know what I mean.

Answer 1

Yes, taking a reference is fine there. You don't intend to give the method shared ownership; it only wants to work with it. You could take a reference for the first case too, since you copy it anyway. But for first case, it takes ownership. There is this trick to still copy it only once:

void ClassA::take_copy_of_sp(boost::shared_ptr<foo> sp) {
    m_sp_member.swap(sp);
}

You should also copy when you return it (i.e not return a reference). Because your class doesn't know what the client is doing with it (it could store a pointer to it and then big bang happens). If it later turns out it's a bottleneck (first profile!), then you can still return a reference.

---

Edit: Of course, as others point out, this only is true if you know your code and know that you don't reset the passed shared pointer in some way. If in doubt, just pass by value.

Answer 2

In the second case, doing this is simpler:

Class::only_work_with_sp(foo &sp)
{    
    ...  
    sp.do_something();  
    ...  
}

You can call it as

only_work_with_sp(*sp);

Answer 3

I would advise against this practice unless you and the other programmers you work with really, really know what you are all doing.

First, you have no idea how the interface to your class might evolve and you want to prevent other programmers from doing bad things. Passing a shared_ptr by reference isn't something a programmer should expect to see, because it isn't idiomatic, and that makes it easy to use it incorrectly. Program defensively: make the interface hard to use incorrectly. Passing by reference is just going to invite problems later on.

Second, don't optimize until you know this particular class is going to be a problem. Profile first, and then if your program really needs the boost given by passing by reference, then maybe. Otherwise, don't sweat the small stuff (i.e. the extra N instructions it takes to pass by value) instead worry about design, data structures, algorithms, and long-term maintainability.

Answer 4

In addition to what litb said, I'd like to point out that it's probably to pass by const reference in the second example, that way you are sure you don't accidentally modify it.

Answer 5

I would avoid a "plain" reference unless the function explicitely may modify the pointer.

A const & may be a sensible micro-optimization when calling small functions - e.g. to enable further optimizations, like inlining away some conditions. Also, the increment/decrement - since it's thread safe - is a synchronization point. I would not expect this to make a big difference in most scenarios, though.

Generally, you should use the simpler style unless you have reason not to. Then, either use the const & consistently, or add a comment as to why if you use it just in a few places.

Answer 6

The point of a distinct shared_ptr instance is to guarantee (as far as possible) that as long as this shared_ptr is in scope, the object it points to will still exist, because its reference count will be at least 1.

Class::only_work_with_sp(boost::shared_ptr<foo> sp)
{
    // sp points to an object that cannot be destroyed during this function
}

So by using a reference to a shared_ptr, you disable that guarantee. So in your second case:

Class::only_work_with_sp(boost::shared_ptr<foo> &sp) //Again, no copy here  
{    
    ...  
    sp->do_something();  
    ...  
}

How do you know that sp->do_something() will not blow up due to a dangling pointer?

It all depends what is in those '...' sections of the code. What if you call something during the first '...' that has the side-effect (somewhere in another part of the code) of clearing a shared_ptr to that same object? And what if it happens to be the only remaining distinct shared_ptr to that object? Bye bye object, just where you're about to try and use it.

So there are two ways to answer that question:

1. Examine the source of your entire program very carefully until you are sure the object won't die during the function body.

2. Change the parameter back to be a distinct object instead of a reference.

General bit of advice that applies here: don't bother making risky changes to your code for the sake of performance until you've timed your product in a realistic situation in a profiler and conclusively measured that the change you want to make will make a significant difference to performance.

Answer 7

It is sensible to pass shared_ptrs by const&. It will not likely cause trouble (except in the unlikely case that the referenced shared_ptr is deleted during the function call, as detailed by Earwicker) and it will likely be faster if you pass a lot of these around. Remember; the default boost::shared_ptr is thread safe, so copying it includes a thread safe increment.

Try to use const& rather than just &, because temporary objects may not be passed by non-const reference. (Even though a language extension in MSVC allows you to do it anyway)

Answer 8

I'll assume that you are familiar with premature optimization and are asking this either for academic purposes or because you have isolated some pre-existing code that is under-performing.

Passing by reference is okay

Passing by const reference is better, and can usually be used, as it does not force const-ness on the object pointed to.

You are not at risk of losing the pointer due to using a reference. That reference is evidence that you have a copy of the smart pointer earlier in the stack and only one thread owns a call stack, so that pre-existing copy isn't going away.

Using references is often more efficient for the reasons you mention, but not guaranteed. Remember that dereferencing an object can take work too. Your ideal reference-usage scenario would be if your coding style involves many small functions, where the pointer would get passed from function to function to function before being used.

You should always avoid storing your smart pointer as a reference. Your Class::take_copy_of_sp(&sp) example shows correct usage for that.

Answer 9

Assuming we are not concerned with const correctness (or more, you mean to allow the functions to be able to modify or share ownership of the data being passed in), passing a boost::shared_ptr by value is safer than passing it by reference as we allow the original boost::shared_ptr to control it's own lifetime. Consider the results of the following code...

void FooTakesReference( boost::shared_ptr< int > & ptr )
{
    ptr.reset(); // We reset, and so does sharedA, memory is deleted.
}

void FooTakesValue( boost::shared_ptr< int > ptr )
{
    ptr.reset(); // Our temporary is reset, however sharedB hasn't.
}

void main()
{
    boost::shared_ptr< int > sharedA( new int( 13 ) );
    boost::shared_ptr< int > sharedB( new int( 14 ) );

    FooTakesReference( sharedA );

    FooTakesValue( sharedB );
}

From the example above we see that passing sharedA by reference allows FooTakesReference to reset the original pointer, which reduces it's use count to 0, destroying it's data. FooTakesValue, however, can't reset the original pointer, guaranteeing sharedB's data is still usable. When another developer inevitably comes along and attempts to piggyback on sharedA's fragile existence, chaos ensues. The lucky sharedB developer, however, goes home early as all is right in his world.

The code safety, in this case, far outweighs any speed improvement copying creates. At the same time, the boost::shared_ptr is meant to improve code safety. It will be far easier to go from a copy to a reference, if something requires this kind of niche optimization.

Answer 10

I would advocate passing shared pointer by const reference - a semantics that the function being passed with the pointer does NOT own the pointer, which is a clean idiom for developers.

The only pitfall is in multiple thread programs the object being pointed by the shared pointer gets destroyed in another thread. So it is safe to say using const reference of shared pointer is safe in single threaded program.

Passing shared pointer by non-const reference is sometimes dangerous - the reason is the swap and reset functions the function may invoke inside so as to destroy the object which is still considered valid after the function returns.

It is not about premature optimization, I guess - it is about avoiding unnecessary waste of CPU cycles when you are clear what you want to do and the coding idiom has firmly been adopted by your fellow developers.

Just my 2 cents :-)