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

Late answer. But it will help to you.

Nowadays, I have been learning C++ and I faced same thinking with you.

When I tested many situation. I can't see equal situation.

But When you use allocator. It would be occur the same.

Here's the code.

#include <iostream>

class Test
{
public:
    int a;
    int b;

    Test(const Test& other) {
        if (this == &other)
            std::cout << "wow!" << std::endl;
        this->a = other.a;
        this->b = other.b;
    }
};

int main()
{
    std::allocator<Test> alloc;
    Test *a = alloc.allocate(sizeof(Test));
    alloc.construct(a, *a);
}

I don't know exactly why it works. But you can see the equal situation.

Answer 2

Writing copy-assignment operators that are safe for self-assignment is in the C++ core guidelines and for a good reason. Running into a self-assignment situation by accident is much easier than some of the sarcastic comments here suggest, e.g. when iterating over STL containers without giving it much thought:

std::vector<Test> tests;
tests.push_back(Test());
tests.resize(10);
for(int i = 0; i < 10; i++)
{
  tests[i] = tests[0]; // self when i==0
}

Does this code make sense? Not really. Can it be easily written through carelessness or in a slightly more complex situation? For sure. Is it wrong? Not really, but even if so... Should the punishment be a segfault and a program crash? Heck no.

To build robust classes that do not segfault for stupid reasons, you must test for self-assignment whenever you don't use the copy-and-swap idiom or some other safe alternative. One should probably opt for copy-and-swap anyway but sometimes it makes sense not to, performance wise. It is also a good idea to know the self-assignment test pattern since it shows in tons of legacy code.

Answer 3

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 4

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

Answer 5

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 6

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.

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

Answer 8

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 9

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.

Answer 10

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 11

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++."