Copy-Assignment Operator when assigns itself

Question

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?

See [the Copy Swap Idiom](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) — chris, Aug 03 '14 at 18:35
The problem is that it doesn't do what is suggested in the quote you posted. Self assignment is not the only issue. If any of the assignment operations fail, you may want the object being assigned to to remain in the state it was before assignment was attempted. — juanchopanza, Aug 03 '14 at 18:38
I've used this approach before. @T.C.: It is most likely in order to avoid the case where you first deallocate previously allocated resources in order to reallocate them according to the attributes of the input argument. — barak manos, Aug 03 '14 at 18:38
@barakmanos I thought it was there to force the compiler to emit an error :-) — juanchopanza, Aug 03 '14 at 18:39
@juanchopanza: Ooos :) ... Should be `if (this == &rhs)`, Which is what I imagined I was seeing... — barak manos, Aug 03 '14 at 18:40
A problem with that approach is that you have a conditional branch each time you want to assign. This could matter if you assign a lot and care about performance. — juanchopanza, Aug 03 '14 at 18:45
@juanchopanza, if I follow the book suggestion, there will be no conditional branch but there will be unnecessary `delete` and `new` during the assignment itself. Isn't it worse than conditional branch? — user3858202, Aug 03 '14 at 19:10
@user3858202 It depends on what you are trying to achieve (and who said anything about `new` and `delete`?) If you want to enforce the strong or even the basic exception guarantee, that has a cost. If you want to protect only against self-assignment, that also has a cost. But in which situation would you want a self assignment not to be an error? Why should you support that? — juanchopanza, Aug 03 '14 at 19:13
@juanchopanza. I edited the question to show `new` and `delete` I mentioned before — user3858202, Aug 03 '14 at 19:26
OK, so if you implement a cheap swap, you can use copy-and-swap at almost no extra cost, and you get exception safety. — juanchopanza, Aug 03 '14 at 19:30

barak manos · Answer 1 · 2014-08-03T18:51:28.360

2

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

edited Aug 03 '14 at 18:51

answered Aug 03 '14 at 18:42

barak manos

29,648
10
62
114

not correct in 100%, in case of exception from new the class C invariant is violated - the C object will be left with truncated array – 4pie0 Aug 03 '14 at 18:52
2

You should put the `new` before the `delete` (and store the result in a local variable), otherwise this code is not exception safe. – Mankarse Aug 03 '14 at 18:52
@Mankarse: The purpose is to explain OP the motivation behind this paradigm, as given to OP by his/her instructor. – barak manos Aug 03 '14 at 18:53
@bits_international: The purpose is to explain OP the motivation behind this paradigm, as given to OP by his/her instructor. – barak manos Aug 03 '14 at 18:53
important part of this paradigm is to assert class invariant – 4pie0 Aug 03 '14 at 18:55
@Mankarse: Would you explain exceptions on the first class of an OO course (C++ for example)? Or would you start with the basics, and then move forward? The purpose here (the way I see it), is trying to understand the context in which the question is asked, and answer it in the details that are relevant to this context. – barak manos Aug 03 '14 at 18:57
@Mankarse: Think of `printf` for example. When they first taught you how to use this function, did they explain everything behind the variable number of arguments, how they are passed to the function in the stack or in registers, how the function loads them into memory, etc? – barak manos Aug 03 '14 at 18:59
@bits_international: Please read the 2 comments above. Thanks. – barak manos Aug 03 '14 at 19:02

score 2 · Answer 2 · answered Aug 03 '14 at 18:42

2

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.

answered Aug 03 '14 at 18:42

Vlad from Moscow

301,070
26
186
335

otherwise there is a problem – 4pie0 Aug 03 '14 at 18:49

score 2 · Answer 3 · edited May 23 '17 at 11:49

2

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.

edited May 23 '17 at 11:49

Community

1
1

answered Aug 03 '14 at 18:48

4pie0

29,204
9
82
118

Wouldn't it be copy and move nowadays? – Deduplicator Aug 03 '14 at 19:38
The default swap will resort to moves, so your Swap should as well http://stackoverflow.com/a/24014460/1141471 – 4pie0 Aug 03 '14 at 19:46
I know the default implementation for swap will use moves, I meant the idiom you refer... – Deduplicator Aug 03 '14 at 19:55
the idiom I refere to is copy-swap idiom – 4pie0 Aug 03 '14 at 20:19

score 2 · Accepted Answer · answered Aug 03 '14 at 19:40

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.

score 0 · Answer 5 · answered May 07 '18 at 10:26

0

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

answered May 07 '18 at 10:26

JiaHao Xu

2,452
16
31

Edward Strange · Answer 6 · 2014-08-04T00:11:56.250

-1

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.

edited Aug 04 '14 at 00:11

answered Aug 03 '14 at 18:36

Edward Strange

40,307
7
73
125

Copy-Assignment Operator when assigns itself

6 Answers6