2

Is this approach unsafe?

#include <tr1/memory>

Foo * createFoo()
{
  return new Foo(5);
}

int main()
{
  std::tr1::shared_ptr<Foo> bar(create());

  return 0;
}

Or would it be preferable for createFoo to return a shared_ptr<Foo> object?

Fred Foo
  • 355,277
  • 75
  • 744
  • 836
tmatth
  • 499
  • 4
  • 14

2 Answers2

3

Your example is safe the way you've written it. However, you could make it even more leak-proof by having your factory method createFoo() return an auto pointer instead of a raw pointer. That way you are guaranteed that there will be no leaks.

So what you'd get is:

#include <memory>
#include <tr1/memory>

std::auto_ptr<Foo> createFoo()
{
  return std::auto_ptr<Foo>(new Foo(5));
}

int main()
{
  std::tr1::shared_ptr<Foo> bar(createFoo());

  return 0;
}

It is of course also possible to have your factory method return a shared_ptr, but this might be seen as overkill, since the returned pointer will generally go out of scope quite quickly, since it will be used in an assignment or constructor. Furthermore, using auto_ptr states the intended use of the pointer more clearly, which is always a plus when people unfamiliar with your code have to understand it.

AVH
  • 11,349
  • 4
  • 34
  • 43
  • Why is it more leakproof? If the `new Foo(5)` fails, it destructs anything constructed and frees the memory. A return of a pointer isn't going to fail, and once it's in the `shared_ptr` it's fine. – David Thornley Apr 07 '11 at 21:30
  • It would be better not using auto_ptr. In this case, returning a simple pointer will be fine. I agree with David. – xis Apr 07 '11 at 21:38
  • 2
    With the simple pointer version, it is possible to do ``Foo * p = createFoo(); p = createFoo();``. If you don't always assign the returned pointer from the factory method to a smart pointer, you can leak memory, with the auto_ptr solution, that's not possible. – AVH Apr 07 '11 at 21:41
  • 1
    Given the TR1 restriction (=no `unique_ptr`), I think returning a `std::auto_ptr` is a good thing. The caller can `.release()` the pointer if he wants, or use it to construct a `shared_ptr`. If C++0x's `unique_ptr` was available I'd prefer that though. +1 – Paul Groke Apr 07 '11 at 23:07
  • @Darhuuk: I'm usually not worried about memory leaks with things like that; a small, simple function can usually be proved correct by examination. If it were a more complicated function, I might go for the `auto_ptr`. – David Thornley Apr 08 '11 at 13:25
  • I would avoid the auto_ptr because it will be deprecated in C++0x. – tmatth Apr 08 '11 at 17:02
  • @tmatth Well, when C++0x comes around fully supported on your compiled, just replace all auto_ptr with unique_ptr. I don't see the problem. – AVH Apr 08 '11 at 20:41
2

The example is safe: if the shared_ptr constructor throws an exception, it delete's its pointer argument before throwing (draft standard, 20.9.11.2.1).

Whether create should return a shared_ptr depends on what its clients may reasonably want to do with its result. If all they ever do is wrap it in a shared_ptr, then return that for extra safety. (Yes, shared_ptr may introduce some coupling.)

Fred Foo
  • 355,277
  • 75
  • 744
  • 836