38

I am not sure about a good way to initialize a shared_ptr that is a member of a class. Can you tell me, whether the way that I choose in C::foo() is fine, or is there a better solution?

class A
{
  public:
    A();
};

class B
{
  public:
    B(A* pa);
};

class C
{
    boost::shared_ptr<A> mA;
    boost::shared_ptr<B> mB;
    void foo();
};

void C::foo() 
{
    A* pa = new A;
    mA = boost::shared_ptr<A>(pa);
    B* pB = new B(pa);
    mB = boost::shared_ptr<B>(pb);
}
Igor
  • 26,650
  • 27
  • 89
  • 114

1 Answers1

31

Your code is quite correct (it works), but you can use the initialization list, like this:

C::C() :
  mA(new A),
  mB(new B(mA.get())
{
}

Which is even more correct and as safe.

If, for whatever reason, new A or new B throws, you'll have no leak.

If new A throws, then no memory is allocated, and the exception aborts your constructor as well. Nothing was constructed.

If new B throws, and the exception will still abort your constructor: mA will be destructed properly.

Of course, since an instance of B requires a pointer to an instance of A, the declaration order of the members matters.

The member declaration order is correct in your example, but if it was reversed, then your compiler would probably complain about mB beeing initialized before mA and the instantiation of mB would likely fail (since mA would not be constructed yet, thus calling mA.get() invokes undefined behavior).


I would also suggest that you use a shared_ptr<A> instead of a A* as a parameter for your B constructor (if it makes senses and if you can accept the little overhead). It would probably be safer.

Perhaps it is guaranteed that an instance of B cannot live without an instance of A and then my advice doesn't apply, but we're lacking of context here to give a definitive advice regarding this.

ereOn
  • 53,676
  • 39
  • 161
  • 238
  • 1
    Given that you're relying on it, it's worth talking about the how the order of construction of members works and the sequence points involved. – CB Bailey Aug 23 '10 at 07:44
  • @ereOn - A return type for a constructor ? – DumbCoder Aug 23 '10 at 07:55
  • @DumbCoder: Copied from the question where it's also wrong. I fixed it. – sbi Aug 23 '10 at 08:30
  • @Charles Bailey: The order of construction actually doesn't matter. If B were constructed first, or even if it were random, it would still be exception-safe. That is a result of non-overlapping construction, not order. Non-overlap implies that at each point where new memory is allocated, previously allocated memory is managed by a fully-constructed subobject. – MSalters Aug 23 '10 at 08:36
  • 4
    @MSalters: If the initializer for `mB` includes the call `mA.get()` (which it does) then the order of initialization very much does matter. – CB Bailey Aug 23 '10 at 08:45
  • 2
    @MSalters: Just to be clear on what I originally meant: while the answer is correct as written, it hasn't been made completely clear why it's correct. If the order of declaration of `mA` and `mB` were reversed in the class definition then it may not be clear why `C() : mA(new A), mB(new B(mA.get())) {}` is incorrect. – CB Bailey Aug 23 '10 at 08:51
  • @sbi: I indeed copied it from the question. Thanks. – ereOn Aug 23 '10 at 08:53
  • @Charles Bailey: Edited my answer so it is more precise regarding the initialization order. (At least I hope so). Feel free to correct it if something seems wrong. Thanks for the suggestion. – ereOn Aug 23 '10 at 09:00
  • I'm with Charles on this. I usually try to prevent for a ctor to rely on the order of members to be initialized. If I can't avoid it, I put a __big and scary comment next to those members' definitions__. – sbi Aug 23 '10 at 09:19
  • The shared_ptr = new MyClass syntax doesn't seem to work for me: #include struct MyClass{}; int main (int argc, char** argv) { boost::shared_ptr MyClassPtr; MyClassPtr = new MyClass; return(0); } I get "no match for operator=". – David Doria Aug 17 '12 at 20:00
  • @DavidDoria: Please post a new question regarding your problem and I will be happy to answer it. Comments on SO are not for asking new questions. – ereOn Aug 20 '12 at 07:21