Avoiding Memory Leaks w/ Exceptions in C++

Question

I allocate objects on the heap and under some circumstances I combine them into a new object (my Foo class mainly contains 2-3 STL containers). (An alternative would be to use copies, but I guess that would be less efficient.)

These operations may fail, thus exceptions may be thrown. Unless I release the dynamic memory whenever I throw an exception, I will have memory leaks.

Would auto_ptr/unique_ptr make sense in this case? I guess so since Combine is a "sink", but what if I wanted to use f1 and f2 after the call to Combine?

Thanks!

Here's my code:

Foo* MakeFoo( )
{
    Foo* foo = 0;

    Foo* f1 = SimpleFoo( );
    if( f1 )
    {
        Foo* f2 = SimpleFoo( );
        if( f2 )
        {
            Foo* combined = Combine( f1, f2 );
            delete f2;
            delete f1;
        }
        else
            foo = f1;
    }
    delete foo;
}

Foo* SimpleFoo( )
{
    Foo* f = 0;

    if( something )
    {
        f = new Foo;
        if( somethingElse )
            throw std::runtime_error( "Error" ); // Memory leak
    }

    return f;
}

Foo* Combine( const Foo* f1, const Foo* f2 )
{
    assert( f1 );
    assert( f2 );

    // Memory leak in MakeFoo( )
    if( something )
        throw std::runtime_error( "Error" );

    Foo* foo = new Foo;

    // Maybe one could also simply modify f1
    foo->Add( *f1 );
    foo->Add( *f2 );

    return foo;
}

Answer 1

Yes, unique_ptr makes sense here. It handles memory management for you, a la RAII. If you want to use the two objects after the call to combine it will work as both are initialized before the call and created outside the scope of the call to Combine.

Answer 2

Would auto_ptr/unique_ptr make sense in this case?

Yes, auto_ptr can be used. It will ensure that there will be no memory leaks.

but what if I wanted to use f1 and f2 after the call to Combine?

You can pass raw pointers to combine function. Sample code can be like below.

auto_ptr<Foo> f1 = auto_ptr<Foo>(new Foo);
auto_ptr<Foo> f2 = auto_ptr<Foo>(new Foo);
Foo* combined = Combine( f1.get(), f2.get() ); 

This way, ownership of pointers will not be transferred to combine function. So, you can use f1 and f2 after combine function also.

Also, make sure you add a catch in MakeFoo function to catch the exceptions thrown by Combine function.

Answer 3

unique_ptr could help here with memory management, but unless you have move support built into your container in Foo::Add you'll want to use a shared_ptr instead because you can't copy the contents of a unique_ptr only move ownership of it.

If your stl has unique_ptr and shared_ptr you probably shouldn't use auto_ptr.

a typedef of shared_ptr will clean up the code some as well.

Answer 4

This answer is going to assume that you meant return foo at the end of MakeFoo(). My first choice would be to refactor so as to not use so much dynamic allocation, along the lines of this:

Foo *MakeFoo(){
  if(!something)
    return 0;

  return Combine(SimpleFoo(), SimpleFoo());
}

Foo SimpleFoo(){
  Foo foo;
  if (something2) // hopefully related to foo. Otherwise, put this condition in MakeFoo
    throw std::runtime_error("Error");

  return foo;
}

Foo *Combine(const Foo &f1, const Foo &f2){
  if (something3)
    throw std::runtime_error("Error");

  Foo *combination = new Foo;
  combination->add(f1);
  combination->add(f2);

  return combination;
}

If that's not an option for you, I would write something like this in '03:

Foo *MakeFoo(){
  auto_ptr<Foo> f1 (SimpleFoo());
  if (!f1.get())
    return 0;

  auto_ptr<Foo> f2> (SimpleFoo());
  if (!f2.get())
    return f1.release();

  Foo *combined = Combine(f1.get(), f2.get());
  f1.release();
  f2.release();
  return combined;
}

Foo *SimpleFoo(){
  if (!something)
    return 0;

  auto_ptr<Foo> f (new Foo);
  if (somethingHopefullyRelatedToF)
    throw std::runtime_error("Error");

  return f.release();
}

unique_ptr can be used identically. shared_ptr is also a very wide hammer that would nevertheless work for this problem. I'm sure the above examples aren't exact fits for you situation, but hopefully they give you ideas that are applicable to your problem.

Answer 5

I've modified Combine and chosen a combination of your anwers to solve this. The code is much shorter and easier to read, too. I'm very satisfied with this, thanks everyone!

typedef std::auto_ptr<Foo> A_Foo;

A_Foo MakeFoo( )
{
    A_Foo foo = SimpleFoo( );
    if( foo.get( ) )
    {
        A_Foo f2 = SimpleFoo( );
        if( f2.get( ) )
            Combine( *foo, *f2 );
    }
    return foo;
}

A_Foo SimpleFoo( )
{
    if( something )
    {
        A_Foo f( new Foo );
        if( somethingElse )
            throw std::runtime_error( "Error" );
        return f;
    }
    else
        return A_Foo( );
}

void Combine( Foo& f1, const Foo& f2 )
{
    if( something )
        throw std::runtime_error( "Error" );

    f1.Add( f2 );
}