Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797283002:
Fixing std::swap(x, x) in base. (Closed)
DescriptionFixing std::swap(x, x) in base.
Discussion:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/ZzfGfgruHxc/discussion
Review-Url: https://codereview.chromium.org/2797283002
Cr-Commit-Position: refs/heads/master@{#462436}
Committed: https://chromium.googlesource.com/chromium/src/+/ed2ec7d0c475caf59f63d1b7620b8939e1eef02a
Patch Set 1 #
Total comments: 11
Patch Set 2 : Reverting pickle's copy assignment. #Patch Set 3 : Removing NOTREACHED from copy assignment. #
Total comments: 4
Patch Set 4 : Nits. #Patch Set 5 : Rebase. #
Messages
Total messages: 33 (13 generated)
Description was changed from ========== Fixing std::swap(x, x) in base. ========== to ========== Fixing std::swap(x, x) in base. Discussion: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/ZzfGfgruHxc/dis... ==========
dyaroshev@yandex-team.ru changed reviewers: + danakj@chromium.org, dcheng@chromium.org
On 2017/04/05 18:35:40, dyaroshev wrote: > mailto:dyaroshev@yandex-team.ru changed reviewers: > + mailto:danakj@chromium.org, mailto:dcheng@chromium.org Fixing std::swap(x, x) in base.
not LGTM all this does is remove dchecks but actually self-assigning would be broken. https://codereview.chromium.org/2797283002/diff/1/base/files/file.cc File base/files/file.cc (right): https://codereview.chromium.org/2797283002/diff/1/base/files/file.cc#newcode74 base/files/file.cc:74: Close(); I don't understand how this wouldn't close the file. https://codereview.chromium.org/2797283002/diff/1/base/pickle.cc File base/pickle.cc (left): https://codereview.chromium.org/2797283002/diff/1/base/pickle.cc#oldcode324 base/pickle.cc:324: memcpy(header_, other.header_, memcpy of overlapping memory is not allowed https://codereview.chromium.org/2797283002/diff/1/base/process/process_posix.cc File base/process/process_posix.cc (left): https://codereview.chromium.org/2797283002/diff/1/base/process/process_posix.... base/process/process_posix.cc:230: other.Close(); How does this not close itself?
On 2017/04/05 19:17:40, danakj wrote:
> not LGTM
>
> all this does is remove dchecks but actually self-assigning would be broken.
It's OK for self-move assignment to destroy the object. It should leave the
object in a valid but unspecified state.
The problem is with std::swap(x, x);
template <typename T>
swap(T& x, T& y) {
auto tmp = std::move(x);
x = std::move(y); // This should not crash but we don't care what it
actually does.
y = std::move(tmp);
}
Here is:
Howard Hinnant (the guy who proposed rvalue references):
https://stackoverflow.com/questions/9322174/move-assignment-operator-and-if-t...
Eric Niebler (noticeable guy in the standards committee)
http://ericniebler.com/2017/03/31/post-conditions-on-self-move/
https://codereview.chromium.org/2797283002/diff/1/base/pickle.cc File base/pickle.cc (left): https://codereview.chromium.org/2797283002/diff/1/base/pickle.cc#oldcode311 base/pickle.cc:311: NOTREACHED(); The post condition for CopyAssignment in N4618 [1] is: t is equivalent to v, the value of v is unchanged [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/n4618.pdf So I think we need to keep this self-assignment check for copy assignment.
Oh I get what I was missing. swap(x,x) does not self-assign with the object |x| until it is already moved-from. So as long as moving from a moved-from thing to itself works (and closing a null thing in that case is fine) we are good.
https://codereview.chromium.org/2797283002/diff/1/base/process/process_posix.cc File base/process/process_posix.cc (left): https://codereview.chromium.org/2797283002/diff/1/base/process/process_posix.... base/process/process_posix.cc:230: other.Close(); On 2017/04/05 19:17:40, danakj wrote: > How does this not close itself? This looks like a broken assignment period. Process a, b; a = std::move(b); would copy b's process_ to a and then close it, closing itself?
https://codereview.chromium.org/2797283002/diff/1/base/pickle.cc File base/pickle.cc (left): https://codereview.chromium.org/2797283002/diff/1/base/pickle.cc#oldcode311 base/pickle.cc:311: NOTREACHED(); On 2017/04/05 20:22:18, dcheng wrote: > The post condition for CopyAssignment in N4618 [1] is: > > t is equivalent to v, the value of v is unchanged > > [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/n4618.pdf > > So I think we need to keep this self-assignment check for copy assignment. This NOTREACHED mislead me - I read it as move assignment. Can I please drop it, it's super weird? https://codereview.chromium.org/2797283002/diff/1/base/pickle.cc#oldcode324 base/pickle.cc:324: memcpy(header_, other.header_, On 2017/04/05 19:17:40, danakj wrote: > memcpy of overlapping memory is not allowed I reverted this file. https://codereview.chromium.org/2797283002/diff/1/base/process/process_posix.cc File base/process/process_posix.cc (left): https://codereview.chromium.org/2797283002/diff/1/base/process/process_posix.... base/process/process_posix.cc:230: other.Close(); On 2017/04/05 20:33:41, danakj wrote: > On 2017/04/05 19:17:40, danakj wrote: > > How does this not close itself? > > This looks like a broken assignment period. > > Process a, b; > a = std::move(b); > > would copy b's process_ to a and then close it, closing itself? Closing actually just assigns a special value to handle. In case of self move assignment it is just closing itself. This seems to be ok.
On 2017/04/05 20:30:30, danakj wrote: > Oh I get what I was missing. swap(x,x) does not self-assign with the object |x| > until it is already moved-from. So as long as moving from a moved-from thing to > itself works (and closing a null thing in that case is fine) we are good. Yeah, took me a while too. This staff is weird)
https://codereview.chromium.org/2797283002/diff/1/base/pickle.cc File base/pickle.cc (left): https://codereview.chromium.org/2797283002/diff/1/base/pickle.cc#oldcode311 base/pickle.cc:311: NOTREACHED(); On 2017/04/05 20:42:47, dyaroshev wrote: > On 2017/04/05 20:22:18, dcheng wrote: > > The post condition for CopyAssignment in N4618 [1] is: > > > > t is equivalent to v, the value of v is unchanged > > > > [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/n4618.pdf > > > > So I think we need to keep this self-assignment check for copy assignment. > > This NOTREACHED mislead me - I read it as move assignment. > Can I please drop it, it's super weird? Dropping the NOTREACHED() seems fine, as long as keep the early return or skip the body.
https://codereview.chromium.org/2797283002/diff/1/base/pickle.cc File base/pickle.cc (left): https://codereview.chromium.org/2797283002/diff/1/base/pickle.cc#oldcode311 base/pickle.cc:311: NOTREACHED(); On 2017/04/05 20:45:49, dcheng wrote: > On 2017/04/05 20:42:47, dyaroshev wrote: > > On 2017/04/05 20:22:18, dcheng wrote: > > > The post condition for CopyAssignment in N4618 [1] is: > > > > > > t is equivalent to v, the value of v is unchanged > > > > > > [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/n4618.pdf > > > > > > So I think we need to keep this self-assignment check for copy assignment. > > > > This NOTREACHED mislead me - I read it as move assignment. > > Can I please drop it, it's super weird? > > Dropping the NOTREACHED() seems fine, as long as keep the early return or skip > the body. Done.
LGTM https://codereview.chromium.org/2797283002/diff/1/base/process/process_posix.cc File base/process/process_posix.cc (left): https://codereview.chromium.org/2797283002/diff/1/base/process/process_posix.... base/process/process_posix.cc:230: other.Close(); On 2017/04/05 20:42:47, dyaroshev wrote: > On 2017/04/05 20:33:41, danakj wrote: > > On 2017/04/05 19:17:40, danakj wrote: > > > How does this not close itself? > > > > This looks like a broken assignment period. > > > > Process a, b; > > a = std::move(b); > > > > would copy b's process_ to a and then close it, closing itself? > > Closing actually just assigns a special value to handle. In case of self move > assignment it is just closing itself. This seems to be ok. > Hrm.. ok ya.. weird. That wasn't what I expected, I read windows code where process_ was a scoped thing, my mistake.
The CQ bit was checked by dyaroshev@yandex-team.ru
LGTM with nits https://codereview.chromium.org/2797283002/diff/40001/base/files/file_unittes... File base/files/file_unittest.cc (right): https://codereview.chromium.org/2797283002/diff/40001/base/files/file_unittes... base/files/file_unittest.cc:114: std::swap(file, file); Slight nit: the standard idiom is: using std::swap; swap(file, file); Might be good to do that, in case someone ever adds a swap(File&, File&) specialization. https://codereview.chromium.org/2797283002/diff/40001/base/values_unittest.cc File base/values_unittest.cc (right): https://codereview.chromium.org/2797283002/diff/40001/base/values_unittest.cc... base/values_unittest.cc:1388: std::swap(test, test); Ditto
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dyaroshev@yandex-team.ru
https://codereview.chromium.org/2797283002/diff/40001/base/files/file_unittes... File base/files/file_unittest.cc (right): https://codereview.chromium.org/2797283002/diff/40001/base/files/file_unittes... base/files/file_unittest.cc:114: std::swap(file, file); On 2017/04/05 21:01:19, dcheng wrote: > Slight nit: the standard idiom is: > > using std::swap; > swap(file, file); > > Might be good to do that, in case someone ever adds a swap(File&, File&) > specialization. Done. https://codereview.chromium.org/2797283002/diff/40001/base/values_unittest.cc File base/values_unittest.cc (right): https://codereview.chromium.org/2797283002/diff/40001/base/values_unittest.cc... base/values_unittest.cc:1388: std::swap(test, test); On 2017/04/05 21:01:19, dcheng wrote: > Ditto Done.
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2797283002/#ps60001 (title: "Nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dyaroshev@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2797283002/#ps80001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491477296006910,
"parent_rev": "3147e853f63020dc085577281385e4aa898a95c6", "commit_rev":
"ed2ec7d0c475caf59f63d1b7620b8939e1eef02a"}
Message was sent while issue was closed.
Description was changed from ========== Fixing std::swap(x, x) in base. Discussion: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/ZzfGfgruHxc/dis... ========== to ========== Fixing std::swap(x, x) in base. Discussion: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/ZzfGfgruHxc/dis... Review-Url: https://codereview.chromium.org/2797283002 Cr-Commit-Position: refs/heads/master@{#462436} Committed: https://chromium.googlesource.com/chromium/src/+/ed2ec7d0c475caf59f63d1b7620b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ed2ec7d0c475caf59f63d1b7620b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
