CppCoreGuidelines
CppCoreGuidelines copied to clipboard
isocpp
C.65 example code is wrong
C.65 Link: C.65: Make move assignment safe for self-assignment
If someone move-assigns an object to itself, line delete ptr; deletes both this->ptr and other.ptr (i.e., temp) since *this and other are the same object. Finally, *this is no longer a valid object.
In my opinion, the move assignment operator must have a self-assignment test. The correct example code is as follows:
// move assignment operator
HasPtr& HasPtr::operator=(HasPtr&& other) noexcept
{
// direct test for self-assignment
if (this == &other) return *this;
// move from other.ptr to this->ptr
T* temp = other.ptr;
other.ptr = nullptr;
delete ptr;
ptr = temp;
return *this;
}
The way I read it the whole point of that example is to show what a bad implementation would look like. However I do think that meaning is difficult to arrive at so clarity could be improved. I wonder if the example is really adding anything. The example above that appears to be the "good" example so I'm not sure another good example would help. That said, the "this line is redundant" comment is a bit confusing as it seems to contradict the rule.
The example specifically says it's showing how to do it without a test, so adding a test to it would break the example. And the rest of the item specifically says you should have the test, so I agree this is a "don't do it this way" example. You need to read the whole item, not just the final example. It should be clarified to avoid that misunderstanding.
@shaneasd
In my opinion, the class Foo's data members are standard-library container types (incl. string) and the int type, they can handle self-assignment elegantly and efficiently. That's why it has the "this line is redundant" comment.
@jwakely
OK, I have understood that C.65 the final example is a "don't do it this way" example.
However, it is too misleading for readers. I think the final example needs to be clearly instructed that this is a "don't do it this way" example.
The wording of this one is a little confusing. The text under 'Reason' implies that x = std::move(x) should never change x. However the stl containers don't give that guarnatee so in general developers should not count on that. The rule should just require that it is "valid but unspecified" like C.64.
The last example is just showing a way to get x = std::move(x) to work without a compare.
There is a disussion about self move at: https://ericniebler.com/2017/03/31/post-conditions-on-self-move/
Editors call: We will update the example to say that this is an example of code that would break without the test.
oops, as @bgloyer pointed out, the example would not break without the test. On self-move, it leaves this->ptr unmodified.
(when delete ptr; executes, this->ptr is null, so it's a no-op, and then it is restored from tmp)
I reverted https://github.com/isocpp/CppCoreGuidelines/commit/7ad62600f1a110d64126a16199d47611ebd00044 in https://github.com/isocpp/CppCoreGuidelines/commit/0b22b82159fe72f3ef93eed3d13ec5531e4099cf