What undefined behaviour does this C++ code contain

Question

I wrote up this code after reading item 11 of Effective C++ ( Third Edition ).

#include <iostream>
using namespace std;

#define MAX_COLORS 20
class Widget
{
 public:
    Widget ( int seed );
    ~Widget ( );
    Widget& operator=( const Widget& rhs );
    void ToString ( );
 private:

    Widget& SelfAssignmentUnsafe ( const Widget& rhs );
    Widget& SelfAssignmentSafe ( const Widget& rhs );
    Widget& SelfAssignmentAndExceptionSafe ( const Widget& rhs );
    void MakeDeepCopy ( const Widget& rhs );
    int *colorPallete;
};

void Widget::ToString()
{
 int i = 0;
 for ( i = 0; i < MAX_COLORS; i++ )
 {
 cout << "colorPallete[" << i << "]: " << colorPallete[i] << endl;
 }
}

Widget::Widget ( int seed ):
    colorPallete ( new int[MAX_COLORS])
    {
     int i = 0;
     for ( i = 0; i < MAX_COLORS; i++ )
     {
      colorPallete[i] = seed + i;
     }
    }

Widget& Widget::operator=( const Widget& rhs )
{
//    return SelfAssignmentUnsafe ( rhs );

//    return SelfAssignmentSafe( rhs ); 

    return SelfAssignmentAndExceptionSafe ( rhs );
}

Widget& Widget::SelfAssignmentUnsafe ( const Widget& rhs )
{
    delete[] colorPallete;
    colorPallete = 0;
    MakeDeepCopy( rhs );
    return *this;
}

Widget& Widget::SelfAssignmentSafe ( const Widget& rhs )
{
    if ( this == &rhs ) return *this;

    delete[] colorPallete;
    colorPallete = 0;
    MakeDeepCopy ( rhs );
    return *this;
}

void Widget::MakeDeepCopy ( const Widget& rhs )
{
    int i = 0;
    colorPallete = new int [MAX_COLORS];
    for ( i = 0;i < MAX_COLORS; i++ )
    {
     colorPallete[i] = rhs.colorPallete[i];
    }
}

Widget& Widget::SelfAssignmentAndExceptionSafe ( const Widget& rhs )
{
    int *origColorPallete = colorPallete;
    MakeDeepCopy ( rhs );
    delete[] origColorPallete;
    origColorPallete = 0;
    return *this;  
}

Widget::~Widget()
{
 delete[] colorPallete;
}    


int main()
{
 Widget b(10);
 Widget a(20);
 b.ToString();
 b = b; 
 cout << endl << "After: " << endl;
 b.ToString();
}

The author talks about handling assignment to self in the assignment operator:

Widget a(10);
a = a;

From the assignment operator for Widget I call Widget::SelfAssignmentAndExceptionSafe.

In Widget::SelfAssignmentAndExceptionSafe the idea is to save the colorPallete pointer in origColorPallete. Then make a deep copy of rhs.colorPallete. When the copy succeeds I delete the original pointer and return reference to self.

The above mechanism is supposed to be self assignment and exception safe.

However, Widget::SelfAssignmentAndExceptionSafe is not able to handle assignment to self properly. The colorPallete array contains junk after self-assignment. Its handling the other cases very well.

Why could this be?

Please help.

[EDIT: After examining all the answers]

Thanks for your answers. I have updated the MakeDeepCopy method and the example's working fine now. Below, I have pasted the updated code:

#include <iostream>

using namespace std;

#define MAX_COLORS 20
class Widget
{
 public:
    Widget ( int seed );
    ~Widget ( );
    Widget& operator=( const Widget& rhs );
    void ToString ( );
 private:
    Widget( Widget& rhs );
    Widget& SelfAssignmentUnsafe ( const Widget& rhs );
    Widget& SelfAssignmentSafe ( const Widget& rhs );
    Widget& SelfAssignmentAndExceptionSafe ( const Widget& rhs );
    void MakeDeepCopy ( const int* rhs );
    int *colorPallete;
};

void Widget::ToString()
{
 int i = 0;
 for ( i = 0; i < MAX_COLORS; i++ )
 {
 cout << "colorPallete[" << i << "]: " << colorPallete[i] << endl;
 }
}

Widget::Widget ( int seed ):
    colorPallete ( new int[MAX_COLORS])
    {
     int i = 0;
     for ( i = 0; i < MAX_COLORS; i++ )
     {
      colorPallete[i] = seed + i;
     }
    }

Widget& Widget::operator=( const Widget& rhs )
{
//    return SelfAssignmentUnsafe ( rhs );

//    return SelfAssignmentSafe( rhs ); 

    return SelfAssignmentAndExceptionSafe ( rhs );
}

Widget& Widget::SelfAssignmentUnsafe ( const Widget& rhs )
{
    delete[] colorPallete;
    colorPallete = 0;
    MakeDeepCopy( rhs.colorPallete );
    return *this;
}

Widget& Widget::SelfAssignmentSafe ( const Widget& rhs )
{
    if ( this == &rhs ) return *this;

    delete[] colorPallete;
    colorPallete = 0;
    MakeDeepCopy ( rhs.colorPallete );
    return *this;
}

void Widget::MakeDeepCopy ( const int* rhs )
{
    int i = 0;
    colorPallete = new int [MAX_COLORS];
    for ( i = 0;i < MAX_COLORS; i++ )
    {
     colorPallete[i] = rhs[i];
    }
}

Widget& Widget::SelfAssignmentAndExceptionSafe ( const Widget& rhs )
{
    int *origColorPallete = colorPallete;
    MakeDeepCopy ( rhs.colorPallete );
    delete[] origColorPallete;
    origColorPallete = 0;
    return *this;  
}

Widget::~Widget()
{
 delete[] colorPallete;
}    


int main()
{
 Widget b(10);
 Widget a(20);
 b.ToString();
 b = b; 
 cout << endl << "After: " << endl;
 b.ToString();
}

[EDIT: Modified code based on Charles's response ]

The idea is to implement "copy-and-swap" idiom to make code both self assignment and exception safe. Note that copy is implemented only in the copy constructor. If the copy succeeds we swap in the assignment operator.

Another improvement over the previous update is that MakeDeepCopy's interface depended on correct usage. We had to store/delete the colorPallete pointer before calling MakeDeepCopy. No such dependencies exist now.

#include <iostream>

using namespace std;

#define MAX_COLORS 20
class Widget
{
 public:
    Widget ( int seed );
    ~Widget ( );
    Widget& operator=( const Widget& rhs );
    void ToString ( );
    Widget( const Widget& rhs );
 private:
    int *colorPallete;
};

void Widget::ToString()
{
 int i = 0;
 for ( i = 0; i < MAX_COLORS; i++ )
 {
 cout << "colorPallete[" << i << "]: " << colorPallete[i] << endl;
 }
}

Widget::Widget ( int seed ):
    colorPallete ( new int[MAX_COLORS])
    {
     int i = 0;
     for ( i = 0; i < MAX_COLORS; i++ )
     {
      colorPallete[i] = seed + i;
     }
    }

Widget::Widget( const Widget& rhs ):
    colorPallete( new int[MAX_COLORS] )
{
    std::copy ( rhs.colorPallete, rhs.colorPallete + MAX_COLORS, colorPallete );
}

Widget& Widget::operator=( const Widget& rhs )
{
    Widget tmp(rhs);

    std::swap ( colorPallete, tmp.colorPallete ); 

    return *this; 
}

Widget::~Widget()
{
 delete[] colorPallete;
}    


int main()
{
 Widget b(10);
 Widget a(20);
 b.ToString();
 b = b; 
 cout << endl << "After: " << endl;
 b.ToString();
}

Answer 1

The junk you're seeing is because the MakeDeepCopy function always copies from the colorPallete member of rhs, instead of the copy you made in origColorPallete.

The following modification would fix it:

int *Widget::MakeDeepCopy ( const int *rhs )
{
    int i = 0;
    int *colorPallete = new int [MAX_COLORS];
    for ( i = 0;i < MAX_COLORS; i++ )
    {
     colorPallete[i] = rhs[i];
    }
    return colorPallete;
}

Widget& Widget::SelfAssignmentAndExceptionSafe ( const Widget& rhs )
{
    int *origColorPallete = colorPallete;
    colorPallete = MakeDeepCopy ( origColorPallete );
    delete[] origColorPallete;
    origColorPallete = 0;
    return *this;        
}

Actually, with the above modification you may want to rename MakeDeepCopy to CopyColorPalette or something (particularly if you want to keep the original MakeDeepCopy for other purposes).

Answer 2

When you call MakeDeepCopy you always pass in the reference to the object. Hence it again runs as a self-assignment.

You'll be much better off if you check against self-assignment in each public method and only run copying if assignment is called with another object passed.

Answer 3

You could avoid a lot of this hassle by simply using a std::vector instead of a dynamically allocated array. Vectors support assignment (including self assignment) so there would be nothing really to do.

Answer 4

the problem is that you are not dealing with the copying on itself. Therefore, when you are performing a copy on itself, the statement

colorPallete = new int [MAX_COLORS];

is actually also overwriting the colorPallete of rhs

Answer 5

Sticking out like a sore thumb in your example is the lack of a user-defined copy constructor. As you are providing a user-defined destructor and assignment operator, it's reasonable to reason that you might need a user-defined copy constructor and that is, indeed, the case here. Any explicit or implicit call to the compiler generated copy constructor will lead to undefined behaviour when the last of the original and the copy is destroyed.

You can write a trivial no-throw swap function for your class and it's fairly easy to write an exception neutral copy constructor. (Actually, I believe that it's trivial to write and reasonably easy to reason that it's exception neutral.) If you implement your assignment operator in terms of these two functions (the copy-and-swap idiom), you should find it a lot easier. In particular, you should find that the need for any checks for self-assignment should go away.

Edit:

Since your update you have made the Widget assignment operator exception safe. However, your design depends on the fact that you only have a single operation in the assignment operation that could possibly throw (the allocation of new memory) as the assigment of ints cannot throw. In general, if you held an array of objects this would not hold.

I understand that MakeDeepCopy is a private function, but even so it has an interface the depends heavily on correct usage. Either the member variable colorPallete must be delete[]ed and set to 0, or it must be saved to a temporary in the event that the call succeeds so that it can then be delete[]ed.

Even if you did not want to make a copy constructor public, I would still use it to implement the assignment operator as it makes the whole code simpler.

E.g.

Widget::Widget( const Widget& rhs )
    : colorPallete( new int[MAX_COLORS] )
{
    // OK because assigning ints won't through
    std::copy( rhs.colorPallete, rhs.colorPallete + MAX_COLORS. colorPallete );
}

Widget& Widget::operator=( const Widget& rhs )
{
    // Try allocating a copy, Widget's copy constructor must
    // leak anything if it throws

    Widget tmp( rhs );

    // If that worked, swap with the copy - this can't throw

    std::swap( colorPallete, tmp.colorPallete );

    // Our old internals are now part of tmp so will be
    // deallocated by tmp's destructor
}

I've got what's effectively your MakeDeepCopy in the copy constructor but without any of the necessary conditions on the calling code because it is a copy constructor and a simple two line assignment operator that is (IMHO) more obviously exception safe.

Note that if you held an array of objects that might throw during assignment, you'd have to do something a bit cleverer to maintain exception safety and transparency. e.g. (and this probably illustrates why using a std::vector is a good idea):

template< class T  >
class PartialArrayDeleter
{
public:
    PartialArrayDeleter( T* p )
        : p_( p ) {}

    ~PartialArrayDeleter()
    {
        delete[] p_;
    }

    void reset()
    {
        p_ = 0;
    }

private:
    T* p_;
};

Widget::Widget( const Widget& rhs )
    : colorPallete( new Obj[MAX_COLORS] )
{
    PartialArrayDeleter<Obj> del( colorPallete );

    std::copy( rhs.colorPallete, rhs.colorPallete + MAX_COLORS. colorPallete );

    del.reset();
}

Edit 2:

In case you think it's not relevant to consider objects other than int being assigned, then note that if you only consider the class that you have, reallocating is not strictly necessary during assignment. All widgets have the same amount of memory allocated in their constructor. A simple, efficient and exception safe assignment operator would be:

Widget& Widget::operator=( const Widget& rhs )
{
    for( size_t i = 0; i != MAX_COLORS; ++i )
    {
        colorPallete[i] = rhs.colorPallete[i];
    }
    return *this;
}

Self-assignment of ints is safe and as previous noted assignment of ints is also exception safe. (I'm not 100% sure but I don't think that std::copy is technically guaranteed safe for a self-assignment copy.)