Self assignment and if (this != &that).

[Денис]

Hi!

In many places assignment operators has an explicit check for self assignment. As far as I know this is considered to be a bad practice. 

Unfortunately I wasn't able to find anything in the style guide about this.

I propose to discuss it and then state the decision in a style guide or at the very least some place that can be easily linked to.

Copy assignment:

Copy assignment should be safe with respect to self assignment - I don't suppose this to be a controversial thing.

However, according to CppCoreGuidelines and some other sources (for example this was one of the problems in Herb Sutter's exceptional c++) - a check  if (this != &that) is an optimisation for a very rear case and should be avoided. I think we should recommend copy and move in cases where default does not work.

Move assignment:

1) It's questionable wether the move assignment should be safe for self assignment at all.

CppCoreGuidelines vote yes, saying that otherwise there will be subtle bugs.

Discussion on stackoverflow reaches the opposite conclusion - the object after self move assignment is in valid but unspecified state.

Scott Meyers (slide 20) writes move assignment that is not safe for self assignment. One of the fundamental ideas here is that it is critical for move to be as fast as possible, and doing something like swap is an extra write.

2) Seems like if (this != &that) check is accepted as a bad idea by everybody.

[Daniel Cheng]

I don't think we wrote it down anywhere, but when we removed the C++03 move emulation macros a few years ago, we removed self-assignment checks and changed them into DCHECKs: https://codereview.chromium.org/1407443002/

The rationale for this was that the argument to the move assignment operator is a temporary--or the caller explicitly asked for it to be treated as one by using std::move(): thus, it's safe to use destructive semantics to take its value. However, in a self-assignment, this precondition is violated, since the argument and this refer to the same object.

Also, I believe this is consistent with STL, which has an explicit guarantee with not checking for self-assignment in move assignment: http://stackoverflow.com/questions/13127455/what-does-the-standard-library-guarantee-about-self-move-assignment.

 

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Денис]

This seems like a reasonable solution. I think stating it somewhere would still be useful. Who do we have to get on board to do this?

I would really like to state that this == &that is also a bad thing.

пятница, 10 марта 2017 г., 19:58:00 UTC+3 пользователь Daniel Cheng написал:

[Matt Giuca]

I believe this pattern comes from the (now very old) C++ FAQ which goes to great pains to explain the problem of not using a self-assignment check, and proposes this as the solution:

https://isocpp.org/wiki/faq/assignment-operators#self-assignment-how

However, note that this explicitly states that "If self-assignment can be handled without any extra code, don’t add any extra code." and "the goal is not to make self-assignment fast. If you don’t need to explicitly test for self-assignment, for example, if your code works correctly (even if slowly) in the case of self-assignment, then do not put an if test in your assignment operator..."

That is echoed by the CppCoreGuidelines, but their "simple" enforcement is not simple at all... because sometimes you do have to add this check, if your objects manage their own pointers.

Generally, on Chromium code:

• Most objects should be non-copyable.
• Classes that are copyable should contain only primitive and self-managing types (i.e., not raw pointers), so should not require a custom assignment operator and not need to explicitly handle self-assignment.
• In rare cases, you will be managing your own resources and therefore need to check for self-assignment (not as an optimization, but for correctness). Mostly these should be in low-level container classes which we probably don't have many left since now we use std containers for most things.

So yeah, generally we should design classes to not need this. But it may be needed in classes that manage their own resources.

[Peter Kasting]

On Fri, Mar 10, 2017 at 10:05 AM, Денис <dyar...@yandex-team.ru> wrote:

This seems like a reasonable solution. I think stating it somewhere would still be useful. Who do we have to get on board to do this?

Our general thinking on style is that we avoid adding to the Google style guide unless there is a situation unique to Chrome that wouldn't be true of Google in general, and thus justifies extra local verbiage.  We make exceptions to that rationale sometimes, but that's the usual starting point.

So the first question would be whether there's anything special about the Chrome codebase here.  I think the answer is no.

The second question would be the breadth and importance of the rule.  It might be worth saying this if every programmer is going to run into it, or it has huge impact.  I think here, again, the answer is no.  The impact of getting this wrong is reasonably small, and since most Chromium code defaults to not providing copy/assignment, it's atypical for people to write this code in most cases.

So my suggestion would be to fix existing usage, and assume people will copy existing usage in most cases if they write new code.  To me that would be sufficient.

If you strongly disagree, then I think the C++ Dos And Donts page (rather than the style guide) would be an appropriate place for this advice, if there's clear sample code on what to do and not do.

PK

[Денис]

I don't think we wrote it down anywhere, but when we removed the C++03 move emulation macros a few years ago, we removed self-assignment checks and changed them into DCHECKs: https://codereview.chromium.org/1407443002/

Ok, I should read my own links better. Here is why we shouldn't do DCHECK:

Some algorithms can do std::swap(x, x);

What it does is:

void swap(T& lhs /* x */, T& rhs /* x */) {
  auto tmp = std::move(lhs);
  lhs = std::move(rhs); // !!! &lhs = &rhs !!!
  rhs = std::move(tmp);
}

std::swap(x, x) should work.

[dan...@chromium.org]

The alternative in these cases would be the move operator= checking if &other == this and aborting if so, because the operator was destructive to other (and itself it they are the same object). Example: https://cs.chromium.org/chromium/src/base/files/file.cc?rcl=a2e16bb6337fc6ddbee61a41c982e8ff471208f0&l=75

I'd prefer DCHECK to that silently breaking, of course. If someone would like to swap() on two files, then they would need to change the operator to handle that case. It sounds like a good best practice to handle self-assignment generally though. So far types that don't handle it trivially and DCHECK haven't needed to support it.

 

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/c9272118-53a6-47ca-9eab-f1f86e5ede6b%40chromium.org.

[Денис]

среда, 5 апреля 2017 г., 1:27:29 UTC+3 пользователь danakj написал:

On Tue, Apr 4, 2017 at 6:19 PM, Денис <dyar...@yandex-team.ru> wrote:

I don't think we wrote it down anywhere, but when we removed the C++03 move emulation macros a few years ago, we removed self-assignment checks and changed them into DCHECKs: https://codereview.chromium.org/1407443002/

Ok, I should read my own links better. Here is why we shouldn't do DCHECK:

Some algorithms can do std::swap(x, x);

What it does is:

void swap(T& lhs /* x */, T& rhs /* x */) {
  auto tmp = std::move(lhs);
  lhs = std::move(rhs); // !!! &lhs = &rhs !!!
  rhs = std::move(tmp);
}

std::swap(x, x) should work.

The alternative in these cases would be the move operator= checking if &other == this and aborting if so, because the operator was destructive to other (and itself it they are the same object). Example: https://cs.chromium.org/chromium/src/base/files/file.cc?rcl=a2e16bb6337fc6ddbee61a41c982e8ff471208f0&l=75

Checking in release if this != other fires extrimly rarely and is generally considered a bad practice. Asserting that this != &that breakes std::swap. I would eliminate all of those and demand that self copy assignment just works and self move assignment lives that in a valid but unspecified state (even if that == this it's ok to self destroy that way).

[Daniel Cheng]

I agree then: I don't think we should try to explicitly handle the self move-assignment case (i.e. we shouldn't have a this != &that check), but it also shouldn't crash: it's ok as long as it's a valid state (even if unspecified).

For the std::swap case, it'll end up working out, because the original value will be in the temporary, which is ultimately moved back.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/5cd207fb-61e3-4c00-bcad-dcb92b10929e%40chromium.org.

[Jan Wilken Dörrie]

To add another view point to the discussion, Eric Niebler (the guy behind the Ranges TS) wrote a blog post about self-move last week: http://ericniebler.com/2017/03/31/post-conditions-on-self-move/ 

He mentions the std::swap(x, x) case and concludes with the following guideline:

Self-moves should do no evil and leave the object in a valid but unspecified state.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAF3XrKr64VNxJv1GXz5YSrMcEmrJJ2399CqzkmbOHAs0J%2B-3ug%40mail.gmail.com.

[Денис]

Yes, his post is based on Howard Hinnant's answer on stack overflow, mentioned in the first post here. This contract (valid but unspecified) is now supported by classes in base (cl).

четверг, 6 апреля 2017 г., 18:53:51 UTC+3 пользователь Jan Wilken Dörrie написал: