Why does my asignment operator not work on self assignments?

Question

It works fine when I do something like a = b but if I do a = a, I get -1.255 +-67 for all elements in the vector. This is my copy constructor and assignment operator:

VecXd(const VecXd &source){
    dimension = source.dimension;
    vector = new T[dimension];
    for(int i=0; i < dimension; i++)
        vector[i] = source.vector[i];
}

VecXd operator=(const VecXd &source){
    dimension = source.dimension;
vector = new T[dimension];
for(int i=0; i < dimension; i++)
    vector[i] = source.vector[i];
return *this;
}

What line are you referring to? – David G Sep 18 '13 at 00:16 — David G, Sep 18 '13 at 00:16
Also note that you're not deleting your array. – zneak Sep 18 '13 at 00:28 — zneak, Sep 18 '13 at 00:28

score 4 · Accepted Answer · answered Sep 18 '13 at 00:28

This is because you are loosing the previous value of source.vector as soon as you allocate a new vector. This is a self-assignment, so source refers to the same object as *this, thus vector and source.vector are one and the same.

You can fix this issue, as well as the memory leak, like this:

VecXd operator=(const VecXd &source){
    dimension = source.dimension;
    T *temp = new T[dimension]; // Don't loose source.vector yet
    for(int i=0; i < dimension; i++)
        temp[i] = source.vector[i];
    delete [] vector; // Delete old vector
    vector = temp;
    return *this;
}

Even better, you can guard against self-assignment to prevent that travesty:

VecXd operator=(const VecXd &source){
    if(this == &source)
        return *this; // This is a self-assignment, so there's nothing to do
    delete [] vector; // Delete old vector
    dimension = source.dimension;
    vector = new T[dimension]; // Now we are sure that vector and source.vector differ
    for(int i=0; i < dimension; i++)
        vector[i] = source.vector[i];
    return *this;
}

Ah, checking for self assignment first; smart. I tried it with just "return" and it said I needed to return a VecXd. I then tried it with "return source" and it works! Thank you! — JakeP, Sep 18 '13 at 00:39

score 3 · Answer 2 · edited May 23 '17 at 11:50

3

If the two vectors are the same, then this line wipes out the contents of the source vector:

vector = new T[dimension];

So the next line where the copy loop occurs, you're reading garbage into garbage. For a truly safe assignment operator/copy constructor use the copy-swap pattern

edited May 23 '17 at 11:50

Community

1
1

answered Sep 18 '13 at 00:21

Doug T.

64,223
27
138
202

No, you are not right. Returning by value will cause a copy of the garbage data but will not cause it. – zneak Sep 18 '13 at 00:27

score 1 · Answer 3 · answered Sep 18 '13 at 00:32

When defining a custom assignment operator, you always have to handle the "self assignment" situation. Small Sample:

Shape& Shape::operator=(const Shape& S) //Assignment operator overloading
{
    if(this==&source)       //Checking for self assignment.
    {       
        return *this;
    }
    m_id=S.m_id;
    return *this;
}

sbabbi · Answer 4 · 2013-09-18T01:09:41.380

Short answer: you are overriding your vector pointer, losing your data (and leaking memory).

The easiest way to implement the assignment operator is the copy-and-swap idiom:

void swap(VecXd & other) {
    using std::swap;
    swap(dimension,other.dimension);
    swap(vector,other.vector);
}

VecXd& operator=(const VecXd &source){
    VecXd cpy (source);
    swap(cpy);
    return *this;
}

This a good choice because:

Self-assignment works automatically
No need for extra delete
If you later add other members to your class, you only need to adjust the copy constructor

Bonus if the swap operator does not throw (as it should), this guarantee the strong exception safety, i.e. if the assignment operator throws (via copy constructor) the state of the object is left unchanged.

score 0 · Answer 5 · answered Sep 18 '13 at 00:39

VecXd operator=(const VecXd &source){
    dimension = source.dimension;
vector = new T[dimension];
for(int i=0; i < dimension; i++)
    vector[i] = source.vector[i];
              ^
             here you assign uninitialized vector values
             to the same vector
return *this;
}

Referring to nonstatic members of objects that have not been constructed or have already been destructed is undefined behavior. Moreover return reference not value:

VecXd& operator=(const VecXd &source){
   // ...
   return *this;
}

this enables chain assignment a=b=c and check for self assignment:

VecXd& operator=(const VecXd &source){
   if(this!=&source) {
       // ... do what needed
   }
   return *this;
}

Why does my asignment operator not work on self assignments?

5 Answers5