Assignment overloading: why does using the same object cause problems in the program?

Question

Suppose we have the following:

class StringClass
{
public:
    ...
    void someProcessing( );
    ...
    StringClass& operator=(const StringClass& rtSide);
    ...
private:
    char *a;//Dynamic array for characters in the string
    int capacity;//size of dynamic array a
    int length;//Number of characters in a
};

StringClass& StringClass::operator=(const StringClass& rtSide)
{
    capacity = rtSide.capacity;
    length = rtSide.length;
    delete [] a;
    a = new char[capacity];

    for (int i = 0; i < length; i++)
       a[i] = rtSide.a[i];

    return *this;
}

My question is: why does this implementation of overloading the assignment operator cause problems when we try to assign an object to itself like:

StringClass s;
s = s;

The textbook I'm reading (Absolute C++) says that after delete [] a; "The pointer s.a is then undefined. The assignment operator has corrupted the object s and this run of the program is probably ruined."

Why has the operator corrupted s? If we're reinitalizing s.a right after we delete it, why does this cause such a problem in the program that we have to redefine the function as:

StringClass& StringClass::operator=(const StringClass& rtSide)
{
    if (this == &rtSide)
    //if the right side is the same as the left side
    {
        return *this;
    }
    else
    {
        capacity = rtSide.capacity;
        length = rtSide.length;
        delete [] a;
        a = new char[capacity];
        for (int i = 0; i < length; i++)
            a[i] = rtSide.a[i];

        return *this;
    }
}

_Why has the operator corrupted `s`?_ When you did `delete [] a;`, you said you no longer have any interest in that memory; you will not attempt to read it or write it. The system may decide to store some housekeeping information in that memory; if you go reading it, you may be reading something different from what was in there before you released it. Even if the system didn't modify it, you're still accessing data you said you no longer have any interest in, so you lied to the compiler, and the compiler hates being lied to and will get its own back. Probably by having your program crash! — Jonathan Leffler, Nov 27 '12 at 02:09

score 5 · Accepted Answer · edited May 23 '17 at 12:12

If you are assigning an object to itself both a and rt.a point to the same string, so when you do delete [] a you are deleting both what a and rt.a point to; then you do reallocate it, but the data you were going to copy (on itself) in the loop has been lost in the delete.

In the loop now you will just copy whatever junk happens to be in the memory returned by new on itself.

By the way, even with the "safety net" of the self-assignment check that assignment operator isn't completely ok (for instance, it's not exception safe); the "safe" way to define the "big three" (copy constructor, assignment operator, destructor) is using the "copy and swap idiom".

score 3 · Answer 2 · answered Nov 27 '12 at 01:26

If you self-assign, you free (delete) the string via the LHS argument before you copy it to the newly allocated space via the RHS argument. This is not a recipe for happiness; it is undefined behaviour and anything may happen. A crash is plausible; if you're really unlucky, it may appear to work.

Corbin · Answer 3 · 2012-11-27T01:31:54.147

Consider what the value of rtSide.a is when you're inside the broken operator=.

It's the same as this->a, the values you just clobbered. Accessing non-owned memory is undefined behavior, thus accessing this->a is undefined behavior (since you just freed it).

delete [] a;
a = new char[capacity];

for (int i = 0; i < length; i++)
   a[i] = rtSide.a[i]; //Invalid when this->a == rtSide.a 
   //because rtSide.a is no longer owned by your program.

If you did actually want to do this, you would have to make a copy of a before deleting it:

char* ca;
if (this == &rtSide) {
    ca = copy of rtSide.a or this->a;
} else {
    ca = rtSide.a;
}

//Do your assigning and whatnot

if (this == &rtSide) {
    delete[] ca;
}

Obviously it's much more efficient to just do nothing instead of making temporary copies of all of an instances own members. It's the same concept as doing int x = 5; int y = x; x = y;

srbhkmr · Answer 4 · 2012-11-27T01:33:41.400

It is because you've first deleted the pointer delete [] a;
and then later on trying to copy from the deleted location:

for (int i = 0; i < length; i++)
       a[i] = rtSide.a[i]; //rtSide has already been deleted as 'this' and '&rtSide' are same.

Remember it is the same location you are trying to copy from, which you've already deleted. Hence, the error!
The later code you posted fixes this problem by checking for self-assignment as a separate case.

score 0 · Answer 5 · answered Nov 27 '12 at 01:28

delete [] a;
a = new char[capacity];

for (int i = 0; i < length; i++)
   a[i] = rtSide.a[i];

That's why. Think of it like this:

You delete whatever a points to, then allocate a new chunk of memory. The new chunk of memory contains garbage which becomes your new data. Do not be confused by the loop that does a[i] = rtSide.a[i]; that only copies the garbage onto itself.

Remember, this and rtSide both lead you to the same object. When you modify the object using this the object that rtSide refers to is modified.

Assignment overloading: why does using the same object cause problems in the program?

5 Answers5