Check for “self-assignment” in copy constructor?

问题:

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
      {
      }
   }
};

回答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.

回答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.

回答3:

If you want to be paranoid, then:

class Test
{
   Test(const Test &copy)
   {
       assert(this != &copy);
       // ...
   }
};

You never want to continue if this == &copy. I've never bothered with this check. The error doesn't seem to frequently occur in the code I work with. However if your experience is different then the assert may well be worth it.

回答4:

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.

回答5:

Your instructor may be thinking of the check for self-assignment in the copy assignment operator.

Checking for self-assignment in the assignment operator is recommended, in both Sutter and Alexandrescu's "C++ Coding Standards," and Scott Meyer's earlier "Effective C++."

回答6:

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;
}

回答7:

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).

回答8:

I agree that self check doesn't make any sense in copy constructor since object isn't yet created but your professor is right about adding the check just to avoid any further issue. I tried with/without self check and got unexpected result when no self check and runtime error if self check exists.

class Test
{    
    **public:**
    Test(const Test& obj )
    {
       size = obj.size;
       a = new int[size];   
    }

    ~Test()
    {....}

    void display()
    {
        cout<<"Constructor is valid"<<endl;
    }
    **private:**

 }

When created copy constructor and called member function i didn

 Test t2(t2);
 t2.display(); 

Output:
Inside default constructor
Inside parameterized constructor
Inside copy constructor
Constructor is valid

This may be correct syntactically but doesn't look right. With self check I got runtime error pointing the error in code so to avoid such situation.

    Test(const Test& obj )
    {
        if(this != &obj )
        {
            size = obj.size;
            a = new int[size];
        }
    }

Runtime Error:
Error in `/home/bot/1eb372c9a09bb3f6c19a27e8de801811': munmap_chunk(): invalid pointer: 0x0000000000400dc0

回答9:

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.