Using swap idiom for exception safety in a constructor

[Alex]

Hi, I want to ensure exception safety inside a constructor by creating a temporary object and then swapping it with *this:

class Test {
public:
Test(int value) : p(0)
{
Test tmp;
tmp.initialize(value); // In case of exception, tmp is destroyed
swap(tmp);
}

~Test()
{
if (p) delete p;
};

swap(Test& other) {...}

private:
Test() : p(0) {}

void initialize(int value)
{
p = new X(value, ...);
// and more members...
}

private:
X * p;
// And many more members...
};

I wanted to know:
a) Is this a common approach?
b) What other options do you recommend when this is not possible (e.g. Test has pure-virtual methods so I can't instantiate the tmp object)?

Thanks

 

[Howard Hinnant]

Hi, I want to ensure exception safety inside a constructor by creating a temporary object and then swapping it with *this:

class Test {
public:
    Test(int value) : p(0)
    {
        Test tmp;
        tmp.initialize(value); // In case of exception, tmp is destroyed
        swap(tmp);
    }

    ~Test()
    {
        if (p) delete p;
    };

    swap(Test& other) {...}

private:
    Test() : p(0) {}

    void initialize(int value)
    {
       p = new X(value, ...);
       // and more members...
    }

private:
    X * p;
    // And many more members...

};

I wanted to know:
a) Is this a common approach?

This looks like a good approach for C++98/03. But I have not seen it
that often.

b) What other options do you recommend when this is not possible (e.g. Test has pure-virtual methods so I can't instantiate the tmp object)?

In C++11 I would do this instead:

Test(int value) : Test()
{
initialize(value); // In case of exception, *this is
destroyed
}

It is basically equivalent to what you have. But it says to default
construct Test first, then initialize it. And if anything goes wrong
during initialization, the destructor will run.

I find the logic simpler to read, once you understand that one
constructor can call another, and that ~Test() runs when *any*
constructor has completed. Another tweak is to mark the default
constructor as noexcept:

Test() noexcept : p(0) {}

But these modifications I discuss are C++11 only. I.e. it requires a
compiler that implements the 2011 C++ standard.

Howard

 

[goran.pusic]

Hi, I want to ensure exception safety inside a constructor by creating a temporary object and then swapping it with *this:

class Test {
public:
Test(int value) : p(0)
{
Test tmp;
tmp.initialize(value); // In case of exception, tmp is destroyed
swap(tmp);
}

~Test()
{
if (p) delete p;
};

swap(Test& other) {...}

private:
Test() : p(0) {}

void initialize(int value)
{
p = new X(value, ...);
// and more members...
}

private:
X * p;
// And many more members...
};

I wanted to know:
a) Is this a common approach?
b) What other options do you recommend when this is not possible (e.g. Test has pure-virtual methods so I can't instantiate the tmp object)?

Thanks

(If I understand you correctly... I presume you have e.g. Y* p2, Z* p3 etc. as a member of Test)

This is not good, and I would hazard it's not common either. It's bad because you just moved the leak from "this" to tmp.Initialize: e.g. if "new p2" fails, p1 is leaked.

One correct approach is e.g.:

class Test
{
X* p1; Y* p2;
public:
Test(A a, B b) : p1(0), p2(0)
{
// can-throw zone here
auto_ptr<X> P1(new X(a)); // unique_ptr in C++11?
auto_ptr<Y> P2(new X(b));
// no-throw zone here
p1 = P1.release();
p2 = P2.release();
}
~Test()
{ // look, ma, no if-s!
delete p1; delete p2;
}
};

In short, use of auto_ptr in the constructor ensures proper cleanup.

 

[Alf P. Steinbach]

Hi, I want to ensure exception safety inside a constructor by creating
a temporary object and then swapping it with *this:

You have achieved a brittle exception safety (not very resistant to code
maintenance) at the cost of general safety.

class Test {
public:
Test(int value) : p(0)
{
Test tmp;
tmp.initialize(value); // In case of exception, tmp is destroyed
swap(tmp);
}

~Test()
{
if (p) delete p;

In passing, note that the `if` here is redundant.

};

swap(Test& other) {...}

In passing, note that it can help others' understanding if you mark the
`swap` as non-throwing, either through C++03 `throw()` or through C++11
`noexcept`, e.g., `swap( Test& other ) throw() { ... }`.

private:
Test() : p(0) {}

void initialize(int value)
{
p = new X(value, ...);
// and more members...
}

private:
X * p;
// And many more members...
};

The major problem here is potential copying of Test objects, whether
through copy construction or through copy assignment.

You will then have two or more objects that both/all think they're
responsible for deleting `p`, and which proceed to do that, with
resulting Undefined Behavior.

In C++98 and C++03 this is called the /rule of three/, that if you need
to define a destructor, a copy constructor or a copy assignment
operator, then you most probably need to define all three.

Or, instead of defining all those operations and making sure that
they're correct, do the sensible thing and use smart pointers and
containers instead of raw pointers and raw arrays.

I wanted to know:
a) Is this a common approach?
No.


b) What other options do you recommend when this is not possible
(e.g. Test has pure-virtual methods so I can't instantiate the tmp object)?

Well, the major option is ordinary C++ RAII throughout, which means:

if you have a data member that requires cleanup, then replace it with
a data member of a class that automates that cleanup.

So let's do that for the `p`.

A first try (intentionally imperfect) might look like this:

class AutoX
{
private:
X* px_;
public:
X* operator->() const { return px_; }

AutoX( int const value )
: px_( new X( value ) )
{}

~AutoX() { delete px_; }
};

The main problem with this class, as in the case of your code, is that
it violates the rule of three: value copying incurs multiple deletions
of the same object, which is Undefined Behavior.

So you need to decide

do you want to support value copying, or to prohibit value copying?

In passing, although not applicable to a class that just wraps a raw
pointer, an alternative to value copying is cloning.

Deciding that you don't need copying, the AutoX class is still simple:

class AutoX
{
private:
X* px_;

AutoX( AutoX const& ); // No such.
AutoX& operator=( AutoX const& ); // No such.

public:
X* operator->() const { return px_; }

AutoX( int const value )
: px_( new X( value ) )
{}

~AutoX() { delete px_; }
};

And then an exception safe Test class becomes just

class Test
{
private:
AutoX p_;
// And many more members...

Test( Test const& ); // No such.
Test& operator=( Test const& ); // No such.

public:
Test( int value )
: p_( value )
// and more members...
{}
};

There is one currently /unconventional feature/ here, namely that
something that behaves like a pointer, namely p, is constructed as it it
were an object of a non-pointer-like class (namely like an X).

I think in the years ahead that will become more and more common,
because C++11 supports general argument forwarding.

However, for C++03 you would have to use something like the forwarding
support I once presented at my blog, or completely manual class-specific
forwarding as in the code above. Neither choice feels totally clean. So
you might instead use a C++03 `std::auto_ptr` (note: as AutoX does not
support copying, and also, is deprecated in C++11):

class Test
{
private:
std::auto_ptr< X > p_;
// And many more members...

Test( Test const& ); // No such.
Test& operator=( Test const& ); // No such.

public:
Test( int value )
: p_( value )
// and more members...
{}
};


Cheers & hth.,

- Alf