Re: [Boost-users] function returns T*, I want to put that into a shared_ptr

From: Carlo Wood (carlo_at_[hidden])
Date: 2006-05-20 12:03:33

On Sat, May 20, 2006 at 02:27:47PM +0800, Josh S wrote:
> class FooContainer {
>    createFoos(int numOfFoos);
>    Foo* getFoo(int indexToFoo);
>    Destroy();
> }
> 
> class BaseContainer {
> 	void addObj(boost::shared_ptr<Base> obj);
> }
> 
> How do I cast the pointer returned from FooContainer::getFoo(), as 
> something I can pass to BaseContainer::addObj ?

  x.addObj(boost::shared_ptr<Foo>(y.getFoo());

However, if addObj doesn't store a shared_ptr internally, the last
shared_ptr to this Foo would be destroyed directly after returning
from this function. That means that the Foo would be destroyed too.

If you want to be sure it sticks around, you need to keep at least
one boost::shared_ptr<Foo> to this object around. For example:

  boost::shared_ptr<Foo> ptr(y.getFoo());
  x.addObj(ptr);
  // Use ptr until you are completely done with this object.
  
  If you need to keep it, always keep a copy of the shared_ptr.

> Is this correct to do this? Assuming I cant change the implementation of 
> FooContainer or the BaseContainer interface.

It seems pretty awkward. The risc seems to be that you destroy
the last shared_ptr (and thus the Foo) at the moment that you still
have a Foo* around.  Therefore, this is only safe when:

1) getFoo() always returns a NEW Foo instance, allocated with new.
2) It doesn't even store a Foo* internally.
3) You turn it immediately into a shared_ptr and use that.

> The BaseContainer interface 
> should not have a special case for adding Foo's.
> 
> Good practise tells me I shouldn't create a temporary either.
> 
> I do not want the shared_ptr in BaseContainer to call delete on Foo (I 
> will manually call FooContainer::Destroy()

Because it is unclear what FooContainer::Destroy() does, it might be
necessary that it is being called, because of side-effects next to
deleting the Foo. That gives a problem though if you already destroyed
the last shared_ptr, then the Foo is already destroyed - so the call
to delete by  FooContainer::Destroy() will result in an undefined core
dump (pun intended). And if you still have a shared_ptr around, then
you can't safely destroy that afterwards, because it would attempt to
delete the Foo object twice.

> Now, what happens if I instead want FooContainer to only be responsible 
> for creating the Foo objects, and not be responsible for deleting them.
> Instead I want the shared_ptr to call delete when the objects reference 
> count reaches 0. How would that done?

Just do

  boost::shared_ptr<Foo> ptr(y.getFoo());

and only work with boost::shared_ptr<Foo> objects from then on.
Never call FooContainer::Destroy, and you should be fine provided
that FooContainer doesn't keep an internal Foo* that it will access
or return on other occassions, and provided that FooContainer::Destroy
doesn't have side effects besides deleting the object.
Somehow I doubt this is possible.

My own solution would be to wrap the Foo* in a separate object and
store that in the FooContainer.

  // Somewhere
  boost::shared_ptr<FooPointer> fp;

  // Elsewhere
  fp = new FooPointer(y, y.getFoo());

  x.addObj(fp);

Then, as soon as you stop using fp because the last shared_ptr<FooPointer>
is destroyed, ~FooPointer should call y.Destroy(Foo*)

(that is, I assume you meant Destroy(Foo*) and not Destroy()...)

Obviously, this isn't automatically safe. You shouldn't return Foo*'s from
FooPointer for example. But at least it helps solve the double delete problem.
I guess that defining FooPointer::operator->() to return a Foo* would be usable though.

-- 
Carlo Wood <carlo_at_[hidden]>