Is it good practice to bind shared pointers returned by functions to lvalue references to const?

Question

Although it took me a while to get used to it, I now grew the habit of letting my functions take shared pointer parameters by lvalue-reference to const rather than by value (unless I need to modify the original arguments, of course, in which case I take them by lvalue-reference to non-const):

void foo(std::shared_ptr<widget> const& pWidget)
//                               ^^^^^^
{
    // work with pWidget...
}

This has the advantage of avoiding an unnecessary copy of a shared pointer, which would mean thread-safely increasing the reference counting and potentially incurring in undesired overhead.

Now I've been wondering whether it is sane to adopt a somewhat symmetrical habit for retrieving shared pointers that are returned by value from functions, like at the end of the following code snippet:

struct X
{
    // ...
    std::shared_ptr<Widget> bar() const
    {
        // ...
        return pWidget;
    }
    // ...
    std::shared_ptr<Widget> pWidget;
};

// ...

// X x;
std::share_ptr<Widget> const& pWidget = x.bar();
//                     ^^^^^^

Are there any pitfalls with adopting such a coding habit? Is there any reason why I should prefer, in general, assigning a returned shared pointer to another shared pointer object rather than binding it to a reference?

Answer 1

This is just a remake of the old question of whether capturing a const reference to a temporary is more efficient than creating a copy. The simple answer is that it isn't. In the line:

// std::shared_ptr<Widget> bar();
std::shared_ptr<Widget> const & pWidget = bar();

The compiler needs to create a local unnamed variable (not temporary), initailize that with the call to bar() and then bind the reference to it:

std::shared_ptr<Widget> __tmp = bar();
std::shared_ptr<Widget> const & pWidget = __tmp;

In most cases it will avoid the creation of the reference and just alias the original object in the rest of the function, but at the end of the day whether the variable is called pWidget or __tmp and aliased won't give any advantage.

On the contrary, for the casual reader, it might look like bar() does not create an object but yield a reference to an already existing std::shared_ptr<Widget>, so the maintainer will have to seek out where bar() is defined to understand whether pWidget can be changed outside of the scope of this function.

Lifetime extension through binding to a const reference is a weird feature in the language that has very little practical use (namely when the reference is of a base and you don't quite care what the exact derived type returned by value is, i.e. ScopedGuard).

Answer 2

You may have the optimization backwards:

struct X
{
    // ...
    std::shared_ptr<Widget> const& bar() const
    {
        // ...
        return pWidget;
    }
    // ...
    std::shared_ptr<Widget> pWidget;
};

// ...

// X x;
std::share_ptr<Widget>  pWidget = x.bar();

As bar is returning a member variable, it must take a copy of the shared_ptr in your version. If you return the member variable by reference the copy can be avoided.

This doesn't matter in both your original version and the version shown above, but would come up if you called:

x.bar()->baz()

In your version a new shared_ptr would be created, and then baz would be called.

In my version baz is called directly on the member copy of the shared_ptr, and the atomic reference increment/decrement is avoided.

Of course the cost of the shared_ptr copy constructor (atomic increment) is very small, and not even noticable in all but the most performance-sensetive applications. If you are writing a performance sensetive application than the better option would be to manage memory manually with a memory pool architecture and then to (carefully) use raw pointers instead.