C.65 'self move' - clarify for stl members

[bgloyer]

Should rule C.65: Make move assignment safe for self-assignment apply if a type has stl members? Rule C.20: If you can avoid defining default operations, do shows an example using stl types without protecting for self-move:

// From the example in C.20 'If you can avoid defining default operations, do'
struct Named_map {
public:
    // ... no default operations declared ...
    string name;
    map<int, int> rep;
};    

int main()
{
    Named_map nm{"Four", {{4,4}}};
    
    cout << "Pre-move:  " << nm.name << ", " << nm.rep[4] << "\n";
    nm = move(nm); // self-move
    cout << "Post-move: " << nm.name << ", " << nm.rep[4] << "\n";    
}

The above outputs:

Pre-move:  Four, 4
Post-move: , 0

which happens becuse stl types generally only guarantee 'valid but unspecified' for self-move. Which should take precedence C.20 or C.65? To me, C.20 should take precedence because the rule-of-zero makes code simpler, self-moves that would cause a problem are likey bugs anyway, and developers may accidently introduce bugs when implemeting a safe self-move.

I made a previous attempt with #1938 but that was too aggresive. Maybe the PR could be reworded to preserve values for self move with an exception for stl members?

[cubbimew]

I think the options for self-moving classes with STL members are

• let them do what their authors believe is right (then "self-move no-op" only applies to pointers etc)
• no-op them too, such as by implementing move as copy-and-swap (and lose rule of zero for many simple classes)

(this was brought up in an older PR, but issue may give it more visibility):

[bgloyer]

What is the next step for this one? If it would be useful, I could throw up a PR for the first bullet and let people throw darts at it.

[HowardHinnant]

The premise of C.65 is fatally flawed. If x = std::move(x) changes the value of x (from one valid value to another), swap(x, x) remains a no-op.

[bgloyer]

The premise of C.65 is fatally flawed. If x = std::move(x) changes the value of x (from one valid value to another), swap(x, x) remains a no-op.

Maybe not fatally flawed. I think the important part of C.65 is that self-move does not double delete, leak, and is no-op for swap(x, x). And the rule covers that. The rule title says 'safe' not 'no-op'. The rule enforecment says pointer members should not be null or deleted but does not require other self-moved memebers to be no-op.

However the text in the body does have the additional constraint that x=move(x) should be no-op.

[HowardHinnant]

I stand by fatally flawed.

The text continually references x = x to the point that I'm not sure if the author knows the difference between copy assignment and move assignment. Is x = move(x) intended? Is this just a type-o? Is there a reason to reference copy assignment in all these places that I'm missing?

The very first sentence:

If x = x changes the value of x, people will be surprised and bad errors can occur.

Why are we talking about copy assignment? Or are talking about move assignment, and x = x is just a type-o?

The entire issue should just be struck thu, and replaced with: Sorry, we made a mistake.

And then a new issue could be written with a different number.

[HowardHinnant]

This sentence:

However, std::swap is implemented using move operations so if you accidentally do swap(a, b) where a and b refer to the same object, failing to handle self-move could be a serious and subtle error.

strongly implies that if self-move-assignment changes the value, then swap(a, a) does the wrong thing. In the context of the paragraph that it appears, I don't see any other reasonable way to interpret it. At the very least this sentence is highly misleading. But the most reasonable interpretation is that it is just flat out wrong.

[poconn]

This page contradicts the rule:

Self-assignment is not valid for move assignment.

I agree with the above arguments against C.65, the swap(a, a) claim is wrong and it breaks the rule of zero when STL types are involved, but if the guidelines are going to commit to it then the above should probably be updated.