Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.c++ > #15215 > unrolled thread

Using swap idiom for exception safety in a constructor

Back to article view | Back to comp.lang.c++

Contents

  Using swap idiom for exception safety in a constructor Alex <acamposr@gmail.com> - 2012-04-03 17:52 -0700
    Re: Using swap idiom for exception safety in a constructor Howard Hinnant <howard.hinnant@gmail.com> - 2012-04-03 19:52 -0700
    Re: Using swap idiom for exception safety in a constructor goran.pusic@gmail.com - 2012-04-04 05:07 -0700
    Re: Using swap idiom for exception safety in a constructor "Alf P. Steinbach" <alf.p.steinbach+usenet@gmail.com> - 2012-04-04 23:19 +0200

#15215 — Using swap idiom for exception safety in a constructor

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

[toc] | [next] | [standalone]

#15216

On Apr 3, 8:52 pm, Alex <acamp...@gmail.com> 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

[toc] | [prev] | [next] | [standalone]

#15219

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.

[toc] | [prev] | [next] | [standalone]

#15222

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

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.c++

csiph-web