Using swap idiom for exception safety in a constructor

Discussion in 'C++' started by Alex, Apr 4, 2012.

  1. Alex

    Alex Guest

    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
    Alex, Apr 4, 2012
    #1
    1. Advertising

  2. On Apr 3, 8:52 pm, Alex <> wrote:
    > 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
    Howard Hinnant, Apr 4, 2012
    #2
    1. Advertising

  3. Alex

    Guest

    On Wednesday, April 4, 2012 2:52:52 AM UTC+2, Alex wrote:
    > 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.
    , Apr 4, 2012
    #3
  4. On 04.04.2012 02:52, Alex wrote:
    > 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
    Alf P. Steinbach, Apr 4, 2012
    #4
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Mac

    idiom for constructor?

    Mac, Jun 1, 2005, in forum: Python
    Replies:
    16
    Views:
    506
    tracyshaun
    Jun 6, 2005
  2. Steve
    Replies:
    10
    Views:
    889
    Matthias Kaeppler
    Jul 23, 2005
  3. Niels Dekker (no reply address)

    What swap is called when using std::swap?

    Niels Dekker (no reply address), Jul 19, 2006, in forum: C++
    Replies:
    4
    Views:
    973
    Niels Dekker (no reply address)
    Jul 20, 2006
  4. Generic Usenet Account
    Replies:
    10
    Views:
    2,194
  5. George2
    Replies:
    4
    Views:
    382
    dizzy
    Jan 8, 2008
Loading...

Share This Page