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

Tangram

I think it must've been your professor's April Fools joke. – Avast

possible duplicate of std::string x(x); – Salic

Your professor likely does not master C++ and should not be teaching it. It is almost never necessary to check for self assignment if you're using the copy-and-swap idiom. [I say "almost" because it is a form of (premature) optimization.]. If you don't know the copy and swap idiom, you must learn it, and learn not to write those creepy code duplicating self assignment testing memory leaking exception unsafe 15-line long assignment operators. – Parliamentarianism

@dialer: why flaw? I don't see any problem with this. – Birkle

If you do the assignment operator using copy and swap there is no need to test for assignment to self either. – Queridas

@ybungalobill Because it enables you to create an object (the second a) without calling any constructor for it (not even default constructor). When you then copy an unconstructed object, all your RAII endeavors will be screwed up. To make it worse, without further checking, the code won't even crash until destruction of the original &copy object. Sure, these assignments are nothing but wrong, but IMO they should also be considered wrong then, by K&R C already. Even copy and swap could screw up device handles etc. – Tangram

@dialer: you have many other ways to create uninitialized objects. E.g: Test a; a.~Test();. So what? – Birkle

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.

Fireboat

That is also what the answers std::string x(x) suggest. I understand what you're saying, but unfortunately it doesn't crash. – Tangram

Yep, and I just wnet and u/v'ed all those answers. – Fireboat

I think the professor is correct although he may not know why he is correct. Consider situations where the object's constructor is called using placement new. – Gibbs

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.

Birkle

It is syntactically valid, but still invalid to use the value before it is initialized. – Mucro

@BoPersson: it does not necessarily use any uninitialized value. I.e. the copy constructor may memset a to 0 without reading anything from the parameter, or it may be an empty class... in such case it will be valid in any possible way from standard POV. – Birkle

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

Comenius

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

Elfredaelfrida

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

Adiabatic

I pointed that out to him as well, which actually made him think a bit. Though, he wasn't entirely sure if the copy constructor needs it or not. – Tangram

If he's really not sure, I would take everything he teaches with a few grains of salt. This is very basic C++ stuff. – Cowhide

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

Agateware

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

Parliamentarianism

Never say always. Never say never either. :-) This answer (#5072582) describes a popular class (std::vector) which would not benefit from an assignment implemented as "copy and swap". – Comenius

@Howard: when you begin C++, it is probably better to say "always do copy and swap" than anything else, at least for the code non duplication and exception safety. The example you describe (thanks for it anyway) is rather contrived, and involves thread safety, which is another bag of worms. – Parliamentarianism

Answer 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

Unrivalled

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

Hiragana

Answer 10

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.

Eared

Answer 11

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.

Homeo