Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1059663002:
Use newer Chromium helper macro for defining move-only type. (Closed)
|
Created:
5 years, 8 months ago by mgraczyk Modified:
5 years, 8 months ago CC:
mojo-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionUse newer Chromium helper macro for defining move-only type.
R=jamesr@chromium.org, viettrungluu@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/b01dd3b35c2c67c6ec8f25e672cd8034dcbf5e91
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add self-assignment check in move assignment operator. #
Total comments: 2
Patch Set 3 : Omit braces #Messages
Total messages: 21 (4 generated)
This commit removes a dependency on an old header in Chromium's move.h that I am changing upstream. BTW, one of the trybots failed on $ ./out/Debug/mojo_shell '--args-for=mojo:dart_apptests ' 'mojo:dart_apptests' I ran the test locally on master and saw sporadic segfaults, occurring roughly 1 in 10 times.
On 2015/04/03 02:53:50, mgraczyk wrote: > This commit removes a dependency on an old header in Chromium's move.h that I am > changing upstream. > > BTW, one of the trybots failed on > > $ ./out/Debug/mojo_shell '--args-for=mojo:dart_apptests ' 'mojo:dart_apptests' > > I ran the test locally on master and saw sporadic segfaults, occurring roughly 1 > in 10 times. To clarify, it remove a dependency on a macro that I am changing. It still uses a different macro in Chromium's move.h.
https://codereview.chromium.org/1059663002/diff/1/mojo/edk/embedder/scoped_pl... File mojo/edk/embedder/scoped_platform_handle.h (right): https://codereview.chromium.org/1059663002/diff/1/mojo/edk/embedder/scoped_pl... mojo/edk/embedder/scoped_platform_handle.h:28: handle_ = other.release(); this operator is incorrect, it doesn't handle self-assignment correctly
https://msdn.microsoft.com/en-us/library/dd293665.aspx is a good guide for how to correctly write move constructors and assignment operators.
https://codereview.chromium.org/1059663002/diff/1/mojo/edk/embedder/scoped_pl... File mojo/edk/embedder/scoped_platform_handle.h (right): https://codereview.chromium.org/1059663002/diff/1/mojo/edk/embedder/scoped_pl... mojo/edk/embedder/scoped_platform_handle.h:28: handle_ = other.release(); On 2015/04/03 17:02:45, jamesr wrote: > this operator is incorrect, it doesn't handle self-assignment correctly Care to elaborate? Here's how I see it. The requirement for self-assignment in a move assignment operator is that it leave the operands on either side of the operator in a valid state. The state itself is unspecified. It can be any valid state, but generally we want self-assignment to be a noop. If we write: ScopedPlatformHandle(h) handle; handle = std::move(handle); What happens is PlatformHandle rv = handle.handle_; handle.handle_ = PlatformHandle(); handle.handle_ = rv; which is a noop. Is that not what you see? I don't mind adding a check for self-assignment. It's more clear that it is correct in that case but I didn't want to change the method body in this CL.
https://codereview.chromium.org/1059663002/diff/1/mojo/edk/embedder/scoped_pl... File mojo/edk/embedder/scoped_platform_handle.h (right): https://codereview.chromium.org/1059663002/diff/1/mojo/edk/embedder/scoped_pl... mojo/edk/embedder/scoped_platform_handle.h:28: handle_ = other.release(); On 2015/04/03 18:30:00, mgraczyk wrote: > On 2015/04/03 17:02:45, jamesr wrote: > > this operator is incorrect, it doesn't handle self-assignment correctly > > Care to elaborate? > > Here's how I see it. The requirement for self-assignment in a move assignment > operator is that it leave the operands on either side of the operator in a valid > state. The state itself is unspecified. It can be any valid state, but > generally we want self-assignment to be a noop. > > If we write: > > ScopedPlatformHandle(h) handle; > handle = std::move(handle); > > What happens is > > PlatformHandle rv = handle.handle_; > handle.handle_ = PlatformHandle(); > handle.handle_ = rv; > > which is a noop. > > Is that not what you see? I don't mind adding a check for self-assignment. > It's more clear that it is correct in that case but I didn't want to change the > method body in this CL. Hmm, good point but I think having an idiomatically correct body would be much clearer. This works today because PlatformHandle is a simple data type but if in the future it becomes something more complex in the future this body will stop working as expected.
https://codereview.chromium.org/1059663002/diff/1/mojo/edk/embedder/scoped_pl... File mojo/edk/embedder/scoped_platform_handle.h (right): https://codereview.chromium.org/1059663002/diff/1/mojo/edk/embedder/scoped_pl... mojo/edk/embedder/scoped_platform_handle.h:28: handle_ = other.release(); On 2015/04/03 19:18:23, jamesr wrote: > On 2015/04/03 18:30:00, mgraczyk wrote: > > On 2015/04/03 17:02:45, jamesr wrote: > > > this operator is incorrect, it doesn't handle self-assignment correctly > > > > Care to elaborate? > > > > Here's how I see it. The requirement for self-assignment in a move assignment > > operator is that it leave the operands on either side of the operator in a > valid > > state. The state itself is unspecified. It can be any valid state, but > > generally we want self-assignment to be a noop. > > > > If we write: > > > > ScopedPlatformHandle(h) handle; > > handle = std::move(handle); > > > > What happens is > > > > PlatformHandle rv = handle.handle_; > > handle.handle_ = PlatformHandle(); > > handle.handle_ = rv; > > > > which is a noop. > > > > Is that not what you see? I don't mind adding a check for self-assignment. > > It's more clear that it is correct in that case but I didn't want to change > the > > method body in this CL. > > Hmm, good point but I think having an idiomatically correct body would be much > clearer. This works today because PlatformHandle is a simple data type but if > in the future it becomes something more complex in the future this body will > stop working as expected. I agree and added the check. Although, too many "idioms" like this lead to code bloat from inlined move constructors...
viettrungluu@chromium.org changed reviewers: + viettrungluu@chromium.org
https://codereview.chromium.org/1059663002/diff/20001/mojo/edk/embedder/scope... File mojo/edk/embedder/scoped_platform_handle.h (right): https://codereview.chromium.org/1059663002/diff/20001/mojo/edk/embedder/scope... mojo/edk/embedder/scoped_platform_handle.h:29: if (this != &other) { nit: for consistency with surrounding code, please omit the braces here
lgtm I think the potential code bloat is something we can safely depend on the toolchain to fix for us.
The CQ bit was checked by mgraczyk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamesr@chromium.org Link to the patchset: https://codereview.chromium.org/1059663002/#ps40001 (title: "Omit braces")
https://codereview.chromium.org/1059663002/diff/20001/mojo/edk/embedder/scope... File mojo/edk/embedder/scoped_platform_handle.h (right): https://codereview.chromium.org/1059663002/diff/20001/mojo/edk/embedder/scope... mojo/edk/embedder/scoped_platform_handle.h:29: if (this != &other) { On 2015/04/03 19:36:51, viettrungluu wrote: > nit: for consistency with surrounding code, please omit the braces here Done.
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because it did not recognize the base URL. Please commit your change manually.
(lgtm too, in case you were wondering)
There's no CQ here: https://github.com/domokit/mojo/blob/master/README.md
On 2015/04/03 19:57:57, jamesr wrote: > There's no CQ here: https://github.com/domokit/mojo/blob/master/README.md "git cl land" isn't working for me because my github account doesn't have commit permission on that repo. Is there a special Chromium account I am supposed to be using? Sorry, I am not super familiar with this commit process and I don't see much about github in the Chromium docs.
What's your github account name?
On 2015/04/03 20:10:21, jamesr wrote: > What's your github account name? mgraczyk https://github.com/mgraczyk
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as b01dd3b35c2c67c6ec8f25e672cd8034dcbf5e91 (presubmit successful). |
