CODEWITHSUNDEEP
c++copy-assignment
user3858202
user3858202

Reputation: 95

Copy-Assignment Operator when assigns itself

I was reading one of the recommended C++ book, on copy-assignment section it suggests;

It is cricially important for assignment operator to work correctly, even when object is assigned to itself. A good way to do so is to copy the right-hand operand before destroying the left hand operand.

the example in the book; class has one data member ps and ps is string *

C& operator=(const C &rhs)
{
     auto newp = new string(*rhs.ps)
     delete ps;
     ps = newp;
     return *this;        
}

but our instructor suggested

C& operator=(const C &rhs)
{
    if (this == &rhs)
        return *this;

     delete ps;
     ps = new string(*rhs.ps)
     return *this;  
}

is there any problem with the instructors' approach?

Upvotes: 0

Views: 1469

Answers (6)

JiaHao Xu
JiaHao Xu

Reputation: 2748

A simple solution: Using std::unique_ptr:

struct C {
    std::unique_ptr<std::string
    C& operator=(const C &rhs)
    {
        ps = new string(*rhs.ps)
        return *this;  
    }
};

Upvotes: 0

Edward Strange
Edward Strange

Reputation: 40849

No. There is no problem with that approach.

Well, now that you edited it there's a problem. If you want good advice I'd suggest giving people full information.

Upvotes: -1

Mankarse
Mankarse

Reputation: 40613

Your instructor's approach is flawed. If new string(*rhs.ps) fails, it will throw an exception, and this will leave ps with an invalid value (ps will be a pointer to deleted memory).

You have to ensure that the new has succeeded before deleteing the old data:

C& operator=(const C &rhs)
{
     auto new_ps = new string(*rhs.ps);
     delete ps;
     ps = new_ps;
     return *this;  
}

You can guard against self assignment if you want to, but it is not needed, and since self-assignment is very much not the common case, doing so will probably reduce the performance of your program.

Remember that if you have a custom copy assignment operator, you probably also want a custom move assignment operator, copy constructor, move constructor and destructor. (See Rule of Five).

Overall, this is still a flawed design. ps should just be a string, or should be a smart pointer such as value_ptr. Manually managing memory is tedious and error-prone.

Upvotes: 2

barak manos
barak manos

Reputation: 30136

You can use the following paradigm in order to avoid the case where you first deallocate previously allocated resources, and then reallocate them according to the attributes of the input argument (which would otherwise result with the undesirable scenario of the input argument resources being deallocated):

C& operator=(const C &rhs)
{
    if (this == &rhs)
        return *this;

    // For example:
    delete[] m_arr;
    m_len = rhs.m_len;
    m_arr = new int[m_len];
    for (int i=0; i<m_len; i++)
        m_arr[i] = rhs.m_arr[i];
}

Upvotes: 2

4pie0
4pie0

Reputation: 29724

is there any problem with this approach?

The problem is that it doesn't do what suggestion tells and in any case it should be

C& operator=( const C &rhs)
{
    if ( this == &rhs)
        return *this;

    // ...
}

and probably the final aim is to write something like this:

C& operator=( C temp)
{
  Swap( temp );
  return *this;
}

see copy swap idiom and consult "Exceptional C++" for more on this topic.

Upvotes: 2

Vlad from Moscow
Vlad from Moscow

Reputation: 310930

There is a problem that the code will not be compiled. Instead of

if (this == rhs.this)

there must be

if (this == &rhs)

otherwise there is no problem.

Upvotes: 2

Related Questions