C.62 etc: Mention copy-and-swap. Eliminate misinformation about self-move-assignment

[Quuxplusone]

Also mention the strong exception guarantee and maintainability; the
"obviously correct" code in the old snippet was in fact NOT correct.

Also fix a ton of grammar issues in these sections.

[shaneasd]

I think the point of the temp variable in the original code was so that you could remember the value of other.ptr after you set it to null. Since std::exchange seems to be designed to do that for you then if you're using std::exchange wouldn't you just write this as

delete ptr;
ptr = std::exchange(other.ptr, nullptr);

or possibly even
delete std::exchange(ptr, std::exchange(other.ptr, nullptr)); ?

Feels like at the moment readability is suffering without any real gain. The same can probably be said of my second example though.

[Quuxplusone]

This snippet is supposed to keep working even if &ptr == &other.ptr, so your two-liner wouldn't work.
Your one-liner seems like it would work fine, though! Totally unreadable, but it does work even when &ptr == &other.ptr as far as I can tell.

However, this entire "Note" seems to be just someone showing off their cleverness; it's not code you'd ever use in real life, given that copy-and-swap already exists. So I certainly wouldn't object to just removing the entire "Note."

[shaneasd]

good point about the two liner. Pure coincidence that the one liner works. I agree the one liner is pretty unreadable. If I were reviewing this code in production I'd be advocating for a comment explaining why it's so sensitive (i.e. easy to get wrong). I have no opinion on whether the example is required.

[cubbimew]

contradicts NR.2 "single-return rule makes it harder to concentrate error checking at the top of a function"

[Quuxplusone]

I doubt that NR.2 was meant to imply "Every function must have at least two return points." Using if for a block of code that you want to be executed conditionally is actually idiomatic C++.

Also, this is a "bad code" example anyway.

[shaneasd]

I don't have an opinion on whether this is better or worse but I don't think "this is a bad code example" is a good justification. a "bad code" example should be bad in precisely one way so that it illustrates the point it is intended to illustrate without any distractions.

[peno2]

It also contradicts F.56
"Shallow nesting of conditions makes the code easier to follow. It also makes the intent clearer. Strive to place the essential code at outermost scope, unless this obscures intent."

"Use a guard-clause to take care of exceptional cases and return early."

[cubbimew]

So you're suggesting to replace the almost invisible costs of a single comparison with an entire copy-and-swap?

[Quuxplusone]

Replace the almost invisible costs of the entire function with the almost invisible costs of a copy-and-swap.

It's true that string::operator= can be faster than string::string(const string&), because the latter must do a heap allocation and a memcpy into the resulting buffer, whereas the former can skip the heap allocation if its buffer is already big enough (but still pays for the memcpy).

QuickBench can be massaged to show various results. The smaller the string is, the better simple-assignment performs. Copy-and-swap beats simple-assignment on libc++; I don't really see why. http://quick-bench.com/YiLlbTxctQKuvRINZVkg1h4MKWw

As far as the C++ Core Guidelines are concerned, though, we're not talking about performance micro-optimizations; we're talking about guidelines that help programmers avoid big correctness issues like

Foo& Foo::operator=(const Foo& a) {
    i = a.i;
    s = a.s;    // ERROR: This code is not exception-safe!
    return *this;
}

The old text went so far as to claim that there was "no known general way of avoiding an if (this == &a) return *this; test for a move assignment and still get a correct answer" — as if copy-and-swap hadn't been known for decades!

[cubbimew]

also NR.2

[cubbimew]

library guarantees the result is valid-but-unspecified (as noted in the next paragraph you're deleting), even though it wasn't specified well in the past, see http://wg21.link/lwg2468 and http://wg21.link/lwg2839 This makes the test redundant in this example.

[Quuxplusone]

In practice, both libc++ and libstdc++ will set s to the empty string. So, if you want foo = std::move(foo) to be a no-op, you must either write special-case code to handle the self-assignment case, or use the copy-and-swap idiom.

https://godbolt.org/z/Bsst2o

Perhaps the Guidelines should go further, and explain that if you follow the Rule of Zero (as in the godbolt above), you will not be following rule C.65, because x = std::move(x) will not be a no-op. The defaulted move assignment operator will not use copy-and-swap; it will use std::string's move-assignment operator, which is not a no-op on self-move.

The test in this example is absolutely not redundant. https://godbolt.org/z/NjL5VR

[cubbimew]

Wait, I didn't realize that you're interpreting C.65 so that foo = std::move(foo) must be a no-op. That's definitely wrong if it gives that impression.

[cubbimew]

...actually it seems to imply it with "x is unchanged", even though it is not supported by C.65's own motivation to make self-swap work (any safe move does). This may be worth a new issue.

[Quuxplusone]

Yep. Right now, rule C.64 says "A move operation should move and leave its source in a valid state" (but potentially any valid state), which is the STL's own rule.

Rule C.65 specifically goes stronger; C.65 says "Make move assignment safe for self-assignment ... If x = x changes the value of x, people will be surprised ... after x = x the value of x is unchanged ... The ISO standard guarantees only a “valid but unspecified” state [but] The rule here is more caution and insists on complete safety."

[cubbimew]

right, and this means the mention of self-swap in the lead is irrelevant, the title is misleading (should be "no-op" rather than "safe"), and that the downsides that you're correctly bringing up were omitted.

[MikeGitb]

Didn't the standard have wording at some point that effectively forbid self move, because the implementation is allowed to assume that, if it has an r-value reference, that reference is the only one referencing that particular object? Has that been cleared up?