Check for “self-assignment” in copy constructor?

Question

Today in university I was recommended by a professor that I'd check for (this != &copy) in the copy constructor, similarly to how you should do it when overloading operator=. However I questioned that because I can't think of any situation where this would ever be equal to the argument when constructing an object.

He admitted that I made a good point. So, my question is, does it make sense to perform this checking, or is it impossible that this would screw up?

Edit: I guess I was right, but I'll just leave this open for a while. Maybe someone's coming up with some crazy cryptic c++ magic.

Edit2: Test a(a) compiles on MinGW, but not MSVS10. Test a = a compiles on both, so I assume gcc will behave somewhat similar. Unfortunately, VS does not show a debug message with "Variable a used without being initialized". It does however properly show this message for int i = i. Could this actually be considered a c++ language flaw?

class Test
{
   Test(const Test &copy)
   {
      if (this != &copy) // <-- this line: yay or nay?
      {
      }
   }
   Test &operator=(const Test &rhd)
   {
      if (this != &rhd) // <-- in this case, it makes sense
      {
      }
   }
};

Answer 1

Personally, I think your professor is wrong and here's why.

Sure, the code will compile. And sure, the code is broken. But that's as far as your Prof has gone with his reasoning, and he then concludes "oh well we should see if we're self-assigning and if we are, just return."

But that is bad, for the same reason why having a global catch-all catch(...) which does nothing is Evil. You're preventing an immediate problem, but the problem still exists. The code is invalid. You shouldn't be calling a constructor with a pointer to self. The solution isn't to ignore the problem and carry on. The solution is to fix the code. The best thing that could happen is your code will crash immediately. The worst thing is that the code will continue in an invalid state for some period of time and then either crash later (when the call stack will do you no good), or generate invalid output.

No, your professor is wrong. Do the assignment without checking for self-assignment. Find the defect in a code review or let the code crash and find it in a debug session. But don't just carry on as if nothing has happened.

Answer 2

This is valid C++ and calls the copy constructor:

Test a = a;

But it makes no sense, because a is used before it's initialized.

Answer 3

In normal situations, it seems like here is no need to. But consider the following situation:

class A{ 
   char *name ; 
public:
   A & operator=(const A & rhs);
};

A & A::operator=(const A &rhs){
   name = (char *) malloc(strlen(rhs.name)+1);
   if(name) 
      strcpy(name,rhs.name);
   return *this;
}

Obviously the code above has an issue in the case when we are doing self assignment. Before we can copy the content, the pointer to the original data will be lost since they both refer to same pointer. And that is why we need to check for self assignment. Function should be like

A & A::operator=(const A &rhs){
     if(this != &rhs){
       name = (char *) malloc(strlen(rhs.name)+1);
       if(name) 
          strcpy(name,rhs.name);
     } 
   return *this;
}

Answer 4

Generally, the operator= and copy constructor calls a copy function, so it is possible that self-assignment occurs. So,

Test a;
a = a;

For example,

const Test& copy(const Test& that) {
    if (this == &that) {
        return *this
    }
    //otherwise construct new object and copy over
}
Test(const &that) {
    copy(that);
}

Test& operator=(const Test& that) {
    if (this != &that) { //not checking self 
        this->~Test();
    }
    copy(that);
 }

Above, when a = a is executed, the operator overload is called, which calls the copy function, which then detects the self assignment.

Answer 5

When writing assignment operators and copy constructors, always do this:

struct MyClass
{
    MyClass(const MyClass& x)
    {
        // Implement copy constructor. Don't check for
        // self assignment, since &x is never equal to *this.
    }

    void swap(MyClass& x) throw()
    {
        // Implement lightweight swap. Swap pointers, not contents.
    }

    MyClass& operator=(MyClass x)
    {
        x.swap(*this); return *this;
    }
};

When passing x by value to the assignment operator, a copy is made. Then you swap it with *this, and let x's destructor be called at return, with the old value of *this. Simple, elegant, exception safe, no code duplication, and no need for self assignment testing.

If you don't know yet about exceptions, you may want to remember this idiom when learning exception safety (and ignore the throw() specifier for swap for now).

Answer 6

Your instructor is probably trying to avoid this situtation -

#include <iostream>

class foo
{
    public:
    foo( const foo& temp )
    {
        if( this != &temp )
        std::cout << "Copy constructor \n";
    }
};

int main()
{
    foo obj(obj);  // This is any how meaning less because to construct
                   // "obj", the statement is passing "obj" itself as the argument

}

Since the name ( i.e., obj ) is visible at the time declaration, the code compiles and is valid.