20

Is the following a reasonable and efficient approach with regard to creating Stuff and giving Foo ownership of it?

class Foo
{
    explicit Foo(const std::shared_ptr<Stuff>& myStuff)
        : m_myStuff(myStuff)
    {
    }

    ...

private:
    const std::shared_ptr<Stuff> m_myStuff;
}

std::shared_ptr<Stuff> foosStuff(new Stuff());
Foo f(foosStuff);
Baz
  • 12,713
  • 38
  • 145
  • 268

3 Answers3

30

Since you are interested in efficiency I'd like to make two points:

shared_ptr<> is one of many standard library types where a move construction is cheaper than a copy construction. Copy constructing shared_ptr is a slower since copying requires the reference counters to be incremented atomically whereas moving a shared_ptr doesn't require touching the referenced data or counters at all. From the article "Want Speed? Pass by value!" by Dave Abrahams one can learn that in certain situations it is actually beneficial to take a function parameter by value. This is one of these cases:

class Foo
{
  explicit Foo(std::shared_ptr<Stuff> myStuff)
  : m_myStuff(move(myStuff))
  {}

  ...

private:
  std::shared_ptr<Stuff> m_myStuff;
};

Now you can write

Foo f (std::make_shared<Stuff>());

where the argument is a temporary and no shared_ptr is ever copied (just moved once or twice).

The use of std::make_shared here has the advantage that only one allocation is done. In your case you allocated the Stuff object by yourself and the shared_ptr constructor had to allocate the reference counters and deleter dynamically as well. make_shared does it all for you with just a single allocation.

sellibitze
  • 27,611
  • 3
  • 75
  • 95
  • 2
    I like that make_shared function – Baz Aug 17 '12 at 08:54
  • 1
    btw: If you change to std::unique_ptr you actually _HAVE_ to pass it by value ;) – sellibitze Aug 17 '12 at 08:56
  • @sellibitze What's the benefit of passing the class a shared_ptr in its constructor if you're not going to keep it. Surely in this case it's be better to pass in a unique_ptr and have the class construct its shared_ptr from that? – jleahy Aug 17 '12 at 09:32
  • 1
    @jleahy: The "benefit" is that the constructor still accepts a shared_ptr while at the same time there are no copying costs involved. Sure, in this usage example I could have used unique_ptr instead. But this might unnecessarily restrict the interface. If you're going to store a shared_ptr anyways, just accept a shared_ptr as parameter, not a unique_ptr. Whether storing a shared_ptr is a good idea is another question and cannot be simply answered without knowing what Baz intended to do here. – sellibitze Aug 17 '12 at 13:22
  • @sellibitze After reading the article you linked to I understand now. If the shared_ptr you're constructing from is a temporary then the compiler will be able to elide the copy completely. – jleahy Aug 17 '12 at 14:55
  • @jleahy: Yes, a modern compiler will do that. But shared_ptr is also move-enabled. So, if the compiler is not able to perform this copy elision for whatever reason, it will at least fall back on a move construction in this case. :) – sellibitze Aug 17 '12 at 15:21
  • 1
    It is important to clarify what version of C++ standard you refer to, otherwise users may mis-interpret the conclusions. So, the answer above is correct as long as you can move-optimise it. It's worth to check Meyers-Alexandrescu-Sutter talk I refer in my answer here http://stackoverflow.com/a/8741626/151641 – mloskot Jan 08 '13 at 12:50
  • @sellibitze I am confused at the use here. The link talks about using this in function calls. Here we are "moving" it to a member variable of a class. That means that it will live on as long as the instance lives...right? So what does "temporary" mean, and is that not affected by this? – ajpieri Dec 01 '22 at 20:37
  • @ajpieri the argument & parameter to the constructor are short-lived `shared_ptr`s. – sellibitze Dec 03 '22 at 10:51
6

Yes, that's perfectly reasonable. This way the work involved in managing the shared pointer will only need to be done once, rather than twice if you passed it by value. You could also consider using make_shared to avoid a copy construction.

std::shared_ptr<Stuff> foosStuff(std::make_shared<Stuff>());

The only improvement you could make is if Foo is to be the sole owner (ie. you're not going to keep foosStuff around after creating Foo) then you could switch to using a std::unique_ptr or boost::scoped_ptr (which is the pre-C++11 equivalent), which would have less overhead.

jleahy
  • 16,149
  • 6
  • 47
  • 66
  • OK, I should look into learning more about other smart pointers as I'm only really familiar with shared_ptr. – Baz Aug 17 '12 at 08:43
  • What is `scoped_ptr`, it's not part of C++? Maybe you mean `std::unique_ptr`? And why the custom deleter? – Christian Rau Aug 17 '12 at 08:54
  • @ChristianRau: I suppose what's mean it `boost::scope_ptr`. The deleter bit may be an outdated reference to an earlier (stealth) edit? – Kerrek SB Aug 17 '12 at 09:00
  • 2
    @KerrekSB I suppose, too, but suppositions don't help anyone reading the answer understand that there is a perfectly suited ownership-taking pointer in the C++ standard library without any need for using boost (which isn't even mentioned). And the question has changed already. Once he notices the question change and inexact information, the downvote on his answer will vanish magically. – Christian Rau Aug 17 '12 at 09:04
  • @ChristianRau My answer has been made slightly redundant by changes to the question. If it wasn't for the custom deleter I would have also preferred make_shared, like in KerrebSB's answer. I've updated it to prevent confusing people. – jleahy Aug 17 '12 at 09:24
  • `make_shared` is not just about saving a (formal) copy. It's actually a more efficient implementation of the type-erased body of the shared pointer. – Kerrek SB Aug 17 '12 at 10:07
3

It might be more efficient to have a make_foo helper:

Foo make_foo() { return Foo(std::make_shared<Stuff>()); }

Now you can say auto f = make_foo();. Or at the very least use the make_shared invocation yourself, since the resulting shared_ptr may be more efficient than the one constructed from a new expression. And if Stuff actually takes constructor arguments, a private auxiliary constructor might be suitable:

struct Foo
{
    template <typename ...Args>
    static Foo make(Args &&... args)
    {
        return Foo(direct_construct(), std::forward<Args>(args)...);
    };

private:

    struct direct_construct{};

    template <typeaname ...Args>
    Foo(direct_construct, Args &&... args)
    : m_myStuff(std::make_shared<Stuff>(std::forward<Args>(args)...))  // #1
    {  }
};

You can either wrap Foo::make into the above make_foo, or use it directly:

auto f = Foo::make(true, 'x', Blue);

That said, unless you're really sharing ownership, a std::unique_ptr<Stuff> sounds like more preferable approach: It is both conceptually much simpler and also somewhat more efficient. In that case you'd say m_myStuff(new Stuff(std::forward<Args>(args)...)) in the line marked #1.

Kerrek SB
  • 464,522
  • 92
  • 875
  • 1,084