Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C.65 example code is wrong #1892

Closed
ltimaginea opened this issue Feb 23, 2022 · 9 comments
Closed

C.65 example code is wrong #1892

ltimaginea opened this issue Feb 23, 2022 · 9 comments
Assignees

Comments

@ltimaginea
Copy link

ltimaginea commented Feb 23, 2022

C.65 Link: C.65: Make move assignment safe for self-assignment

image

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;
}
@shaneasd
Copy link
Contributor

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.

@jwakely
Copy link
Contributor

jwakely commented Feb 23, 2022

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.

@ltimaginea
Copy link
Author

@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.

@ltimaginea
Copy link
Author

@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.

@cubbimew
Copy link
Member

see also #1649

@bgloyer
Copy link
Contributor

bgloyer commented Feb 28, 2022

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/

@hsutter
Copy link
Contributor

hsutter commented Apr 14, 2022

Editors call: We will look to see if we can make this clearer.

@hsutter
Copy link
Contributor

hsutter commented Aug 8, 2022

Editors call: We will update the example to say that this is an example of code that would break without the test.

@cubbimew
Copy link
Member

cubbimew commented Oct 7, 2022

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 7ad6260 in 0b22b82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants