The correct way to write auto-assigment-safe copy functions and why? C++

Question

Im reading a book about c++ and in the "Copy Control" section the author teach us to write the operator= in a class telling us that we have to be sure that the method is safe of self-assigment when we have a class that use dynamic memory.

So, imagine that we have a class named "Bank_Client" with a std::string in created with new. The book teach us to do this to avoid the self-assigment case:

Bank_Client& Bank_Client::operator=(const Bank_Client &addres){
    std::string *temp = new std::string(*addres.name);
    delete name;
    name = temp;
    return *this;
}

So if i do

Bank_Client bob("Bobinsky");
bob = bob;

The program will not just blow-up. But right when i thought that temp variable was a waste of time the writer of the book show us another way to do it:

Bank_Client& Bank_Client::operator=(const Bank_Client &addres){
    if (this != &addres){
        delete name;
        name = new std::string(*addres.name);
    }
    return *this;
}

Like if he read my mind. BUT right after that he tell us to never do that, that is better to do it by the other way but never explain why.

Why is the first way better? It is slower, isn't it? What is the best why to do it?

what about use assert to check there is no self-assignment? (because we dont really need it). And then desactivate it with the corresponding NDEBUG then there is no waste of time in the checking.

It's seems the book is stuipid if it recommends `new std::string` — 273K, Feb 23 '19 at 08:00
Is not about creating a std::string with new (because a std::string is created with dynamic memory, yes, we all know) Its just my example to not make a 100 lines code in the forum:) The book is from 2011 — Jason Bobinsky, Feb 23 '19 at 08:03
There are countless garbage C++ books out there, [this is a list of good books](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list/388282#388282). — Passer By, Feb 23 '19 at 10:18
Ty, i seen that list for buy this book, because is the first one in the list (Primer) — Jason Bobinsky, Feb 23 '19 at 18:07

score 1 · Answer 1 · answered Feb 23 '19 at 07:59

1

The first way is slower if an object is being self-assigned. However, self-assignment is rare. In all other cases, the additional if check from the second approach is a waste.

That said, both approaches are poor ways to implement the copy-assignment operator: neither is exception-safe. If the assignment fails partway through, you'll be left with a half-assigned object in some inconsistent state. It's also bad that it partly duplicates logic from the copy-constructor. Instead, you should implement the copy-assignment operator using the copy-and-swap idiom.

answered Feb 23 '19 at 07:59

jamesdlin

81,374
13
159
204

what about use `assert` to check there is no self-assignment? (because we dont really need it). And then desactivate it with the corresponding `NDEBUG` then there is no waste of time. – Jason Bobinsky Feb 23 '19 at 08:17
You could use `assert`, but why bother? I would still strongly recommend against that. 1. You might not think you'll need to handle self-assignment *now*, and even though it's rare, it might still happen someday when you don't expect it. 2. You *really* should use copy-and-swap instead, which implicitly handles self-assignment for you. Why not use that? – jamesdlin Feb 23 '19 at 08:23
One reason: Between the option of the if-check, the option of the "temp" variable and the swap option (descarting the assert option). Isn't the swap option the slower one? Isn't a waste of time create a copy by value of the variable and then assign `this` to that temporal `Bank_Client` class that will be deleted instantly? As a C++ programmer we should consider the speed more than important – Jason Bobinsky Feb 23 '19 at 18:17
@JasonBobinsky Even without copy-and-swap, you still need to effectively do member-wise copying and destruction. Additional overhead from swapping internal pointers should be negligible and well worth the benefit of better correctness and reduced maintenance costs. Furthermore, [a properly implemented copy-and-swap implementation of copy-assignment](https://web.archive.org/web/20140205194657/http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/) can allow the copy to be elided anyway. – jamesdlin Feb 23 '19 at 18:23

score 0 · Accepted Answer · answered Feb 23 '19 at 08:10

You should use copy and swap. To that end you need a copy constructor (and maybe a move contructor as well if you want to also use move semantics).

class Bank_Client {
    // Note that swap is a free function:
    // This is important to allow it to be used along with std::swp
    friend void swap(Bank_Client& c1, Bank_Client& c2) noexcept {
        using std::swap;
        swap(c1.name, c2.name);
        // ...
    }
    // ...
};

// Note that we took the argument by value, not const reference
Bank_Client& Bank_Client::operator=(Bank_Client address) {
    // Will call the swap function we defined above
    swap(*this, adress);
    return *this;
}

Now, let's take a look at the client code:

Bank_Client bob("Bobinsky");
// 1. Copy constructor is called to construct the `address` parameter of `operator=()`
// 2. We swap that newly created copy with the current content of bob
// 3. operator=() returns, the temporary copy is destroyed, everything is cleaned up!
bob = bob;

Obviously, you make a useless copy when doing self assignment, but it has the merit of allowing you to reuse logic in your copy constructor (or move constructor) and it is exception safe: nothing is done if the initial copy throws an exception.

The other benefit is you only need one implementation for operator=() to handle both copy and move semantics (copy-and-swap and move-and-swap). If the performance is a big issue you can still have an rvalue overload operator=(Bank_Client&&) to avoid the extra move (though I discourage it).

As a final word, I would also advise you to try to rely on the rule of 0 to the best of your ability as in the example above, should the content of the class change, you must also update the swap function accordingly.

Isn't the assigment from `this` to the temporal (copied by value) address a waste of time? — Jason Bobinsky, Feb 23 '19 at 08:44
@JasonBobinsky Yes it is, very much like the useless copy of `name` in the first implementation you proposed. Note that this useless copy only happens when doing a self assignment (which should almost never occur in real life code). PS: I assumed you talked about the copy made to initialize `address` since there is no assignment in the `operator=`, only a `swap`) — Rerito, Feb 23 '19 at 08:50
And in comparation with the first code? What about using and "assert" instead of and if? — Jason Bobinsky, Feb 23 '19 at 08:54
I mean, the code that makes just a `if` comparation. That was my first doubt — Jason Bobinsky, Feb 23 '19 at 09:16
@JasonBobinsky First, `assert` is a noop when you compile in release configurations so it does nothing. Then, in debug, it crashes your program. You don't want to forbid self-assignment so just go with the `if` — Rerito, Feb 23 '19 at 10:13

The correct way to write auto-assigment-safe copy functions and why? C++

2 Answers2