Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1699773002:
Extend base::Callback to have a move-only variant (Closed)
DescriptionExtend base::Callback to have a move-only variant
Add CopyMode flag to base::Callback to split it to the copyable variant
and the move-only variant.
BUG=554299
Committed: https://crrev.com/77d41139d261342a429d2775c59d8e8a386d4c81
Cr-Commit-Position: refs/heads/master@{#380108}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : +msvc work around #Patch Set 4 : msvc work around #
Total comments: 5
Patch Set 5 : #
Total comments: 4
Patch Set 6 : rebase. +comment. #Patch Set 7 : shrink binary size #Patch Set 8 : attempt #2 #Patch Set 9 : attempt #3 #Patch Set 10 : fix android build #
Total comments: 9
Patch Set 11 : +explicit #
Total comments: 2
Patch Set 12 : +explicit again. move BASE_EXPORT to .h #
Total comments: 2
Patch Set 13 : #
Total comments: 1
Patch Set 14 : rebase #
Messages
Total messages: 68 (27 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Description was changed from ========== Extend base::Callback to have move-only variant Add CopyMode flag to base::Callback to split it to the copyable variant and the move-only variant. BUG= ========== to ========== Extend base::Callback to have a move-only variant Add CopyMode flag to base::Callback to split it to the copyable variant and the move-only variant. BUG= ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/40001
Description was changed from ========== Extend base::Callback to have a move-only variant Add CopyMode flag to base::Callback to split it to the copyable variant and the move-only variant. BUG= ========== to ========== Extend base::Callback to have a move-only variant Add CopyMode flag to base::Callback to split it to the copyable variant and the move-only variant. BUG=554299 ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
tzik@chromium.org changed reviewers: + dcheng@chromium.org, thakis@chromium.org
PTAL
filed https://code.google.com/p/chromium/issues/detail?id=586967 for the red build
Cool. Is the idea that the type is a transitional thing, or will we have both types indefinitely? What's the UI for creating a move-only callback – do I explicitly pass the MoveOnly flag to some function? If so, how will both kinds behave if I a) copy the callback b) call it more than once? https://codereview.chromium.org/1699773002/diff/60001/base/callback.h File base/callback.h (left): https://codereview.chromium.org/1699773002/diff/60001/base/callback.h#oldcode356 base/callback.h:356: // please include "base/callback_forward.h" instead. This paragraph could maybe stay? https://codereview.chromium.org/1699773002/diff/60001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/1699773002/diff/60001/base/callback.h#newcode373 base/callback.h:373: this->set_bind_state(bind_state); i'm a bit surpised the this-> wasn't necessary before but is now…oh, i see, the base is now a dependent type. nevermind :-) https://codereview.chromium.org/1699773002/diff/60001/base/callback_forward.h File base/callback_forward.h (right): https://codereview.chromium.org/1699773002/diff/60001/base/callback_forward.h... base/callback_forward.h:13: MoveOnly, Copyable, Please give these two a comment explaining what they mean each.
On 2016/02/15 21:47:57, Nico wrote: > Cool. Is the idea that the type is a transitional thing, or will we have both > types indefinitely? What's the UI for creating a move-only callback – do I > explicitly pass the MoveOnly flag to some function? If so, how will both kinds > behave if I a) copy the callback b) call it more than once? > > https://codereview.chromium.org/1699773002/diff/60001/base/callback.h > File base/callback.h (left): > > https://codereview.chromium.org/1699773002/diff/60001/base/callback.h#oldcode356 > base/callback.h:356: // please include "base/callback_forward.h" instead. > This paragraph could maybe stay? > > https://codereview.chromium.org/1699773002/diff/60001/base/callback.h > File base/callback.h (right): > > https://codereview.chromium.org/1699773002/diff/60001/base/callback.h#newcode373 > base/callback.h:373: this->set_bind_state(bind_state); > i'm a bit surpised the this-> wasn't necessary before but is now…oh, i see, the > base is now a dependent type. nevermind :-) > > https://codereview.chromium.org/1699773002/diff/60001/base/callback_forward.h > File base/callback_forward.h (right): > > https://codereview.chromium.org/1699773002/diff/60001/base/callback_forward.h... > base/callback_forward.h:13: MoveOnly, Copyable, > Please give these two a comment explaining what they mean each. It's transitional. We'll eventually replace all existing Callbacks with move-only ones and remove CopyMode. I'll add two other flags, RepeatMode and ThreadAffinity, that will be permanent, OTOH. For UI, I'll add MoveOnlyCallback as an alias for users once other stuff are ready: move-only type support for Callback::Run and Bind, and MoveOnlyCallback version of Bind. For copyability and multiple call: both variants can be called more than once at this point, I'll split move-only version into OneShot and Repeating later. The behavior on copy and multiple call is the same to the existing Callback, the internal state is shared and used for each invocation.
https://codereview.chromium.org/1699773002/diff/60001/base/callback.h File base/callback.h (left): https://codereview.chromium.org/1699773002/diff/60001/base/callback.h#oldcode356 base/callback.h:356: // please include "base/callback_forward.h" instead. On 2016/02/15 21:47:57, Nico wrote: > This paragraph could maybe stay? Done. https://codereview.chromium.org/1699773002/diff/60001/base/callback_forward.h File base/callback_forward.h (right): https://codereview.chromium.org/1699773002/diff/60001/base/callback_forward.h... base/callback_forward.h:13: MoveOnly, Copyable, On 2016/02/15 21:47:57, Nico wrote: > Please give these two a comment explaining what they mean each. Added a brief comment.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1699773002/diff/80001/base/callback_internal.h File base/callback_internal.h (left): https://codereview.chromium.org/1699773002/diff/80001/base/callback_internal.... base/callback_internal.h:86: explicit CallbackBase(BindStateBase* bind_state); Why change this from initializing in the ctor to using a setter?
https://codereview.chromium.org/1699773002/diff/80001/base/callback_internal.h File base/callback_internal.h (left): https://codereview.chromium.org/1699773002/diff/80001/base/callback_internal.... base/callback_internal.h:86: explicit CallbackBase(BindStateBase* bind_state); On 2016/02/16 23:12:26, dcheng wrote: > Why change this from initializing in the ctor to using a setter? That is a workaround of a MSVC 2013 bug. It doesn't recognize a dependent type as a parent class, so CallbackBase<copy_mode>(bind_state) in Callback<> initializer fails. Adding a comment there.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1699773002/diff/80001/base/callback_internal.h File base/callback_internal.h (left): https://codereview.chromium.org/1699773002/diff/80001/base/callback_internal.... base/callback_internal.h:86: explicit CallbackBase(BindStateBase* bind_state); On 2016/02/17 at 00:46:56, tzik wrote: > On 2016/02/16 23:12:26, dcheng wrote: > > Why change this from initializing in the ctor to using a setter? > > That is a workaround of a MSVC 2013 bug. It doesn't recognize a dependent type as a parent class, so CallbackBase<copy_mode>(bind_state) in Callback<> initializer fails. > Adding a comment there. Just curious, but does this have any effect on binary size?
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/120001
On 2016/02/29 17:51:58, dcheng wrote: > https://codereview.chromium.org/1699773002/diff/80001/base/callback_internal.h > File base/callback_internal.h (left): > > https://codereview.chromium.org/1699773002/diff/80001/base/callback_internal.... > base/callback_internal.h:86: explicit CallbackBase(BindStateBase* bind_state); > On 2016/02/17 at 00:46:56, tzik wrote: > > On 2016/02/16 23:12:26, dcheng wrote: > > > Why change this from initializing in the ctor to using a setter? > > > > That is a workaround of a MSVC 2013 bug. It doesn't recognize a dependent type > as a parent class, so CallbackBase<copy_mode>(bind_state) in Callback<> > initializer fails. > > Adding a comment there. > > Just curious, but does this have any effect on binary size? Good point. This gains the stripped binary size on Linux by 150kB, which is 0.13% of chrome binary. Trying to shrink it..
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
Updated! Linux bot failure looks a dependency issue, that is not directly related to this CL. Some files were not rebuilt. https://codereview.chromium.org/1699773002/diff/80001/base/callback_internal.h File base/callback_internal.h (left): https://codereview.chromium.org/1699773002/diff/80001/base/callback_internal.... base/callback_internal.h:86: explicit CallbackBase(BindStateBase* bind_state); On 2016/02/29 17:51:58, dcheng wrote: > On 2016/02/17 at 00:46:56, tzik wrote: > > On 2016/02/16 23:12:26, dcheng wrote: > > > Why change this from initializing in the ctor to using a setter? > > > > That is a workaround of a MSVC 2013 bug. It doesn't recognize a dependent type > as a parent class, so CallbackBase<copy_mode>(bind_state) in Callback<> > initializer fails. > > Adding a comment there. > > Just curious, but does this have any effect on binary size? Done. Now, the stripped binary size on Linux is the same to the previous size.
https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal... base/callback_internal.cc:21: BASE_EXPORT CallbackBase<CopyMode::MoveOnly>::CallbackBase(CallbackBase&& c) Huh. It's surprising to see _EXPORT macros in a .cc file. https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal... base/callback_internal.cc:63: } Can we default the move operators? It seems like this should work: $ cat test.cc #include <iostream> #include <utility> struct MoveOnly { explicit MoveOnly(int x) : x(x) { } MoveOnly(MoveOnly&& rhs) : x(0) { using std::swap; swap(x, rhs.x); } MoveOnly& operator=(MoveOnly&& rhs) { using std::swap; swap(x, rhs.x); } int x; }; struct Copyable : MoveOnly { explicit Copyable(int x) : MoveOnly(x) { } Copyable(const Copyable& rhs) : Copyable(x) { } Copyable(Copyable&&) = default; Copyable& operator=(Copyable&&) = default; }; int main() { Copyable a(2); Copyable b(3); std::cout << "a: " << a.x << ", b: " << b.x << "\n"; a = std::move(b); std::cout << "a: " << a.x << ", b: " << b.x << "\n"; Copyable c(std::move(a)); std::cout << "a: " << a.x << ", c: " << c.x << "\n"; return 0; } $ ./a.out a: 2, b: 3 a: 3, b: 2 a: 0, c: 3 Also minor nit that these should be in the same order as they are in the header. https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal... base/callback_internal.h:92: CallbackBase(BindStateBase* bind_state); Just curious: why can this no longer be explicit?
https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal... base/callback_internal.cc:63: } On 2016/03/02 08:02:36, dcheng wrote: > Can we default the move operators? > > It seems like this should work: > $ cat test.cc > #include <iostream> > #include <utility> > > struct MoveOnly { > explicit MoveOnly(int x) : x(x) { } > MoveOnly(MoveOnly&& rhs) : x(0) { > using std::swap; > swap(x, rhs.x); > } > MoveOnly& operator=(MoveOnly&& rhs) { > using std::swap; > swap(x, rhs.x); > } > int x; > }; > > struct Copyable : MoveOnly { > explicit Copyable(int x) : MoveOnly(x) { > } > Copyable(const Copyable& rhs) : Copyable(x) { > } > Copyable(Copyable&&) = default; > Copyable& operator=(Copyable&&) = default; > }; > > int main() { > Copyable a(2); > Copyable b(3); > std::cout << "a: " << a.x << ", b: " << b.x << "\n"; > a = std::move(b); > std::cout << "a: " << a.x << ", b: " << b.x << "\n"; > Copyable c(std::move(a)); > std::cout << "a: " << a.x << ", c: " << c.x << "\n"; > return 0; > } > > $ ./a.out > a: 2, b: 3 > a: 3, b: 2 > a: 0, c: 3 > > Also minor nit that these should be in the same order as they are in the header. We are still using MSVC2013, which does not support defaulted move constructor. MSVC2015 seems to support it and hopefully the compiler switch will happend this week. https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal... base/callback_internal.h:92: CallbackBase(BindStateBase* bind_state); On 2016/03/02 08:02:36, dcheng wrote: > Just curious: why can this no longer be explicit? Oh, I just overlook this. I forgot to add it when I add back "BindStateBase* bind_state".
https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal... base/callback_internal.cc:21: BASE_EXPORT CallbackBase<CopyMode::MoveOnly>::CallbackBase(CallbackBase&& c) On 2016/03/02 08:02:36, dcheng wrote: > Huh. It's surprising to see _EXPORT macros in a .cc file. Each clang, gcc and cl.exe behave differently around the symbol visibility and explicit template instantiation... I don't know what the best practice is. https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal... base/callback_internal.h:92: CallbackBase(BindStateBase* bind_state); On 2016/03/02 08:02:36, dcheng wrote: > Just curious: why can this no longer be explicit? Done.
https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal... base/callback_internal.cc:21: BASE_EXPORT CallbackBase<CopyMode::MoveOnly>::CallbackBase(CallbackBase&& c) On 2016/03/02 at 13:09:52, tzik wrote: > On 2016/03/02 08:02:36, dcheng wrote: > > Huh. It's surprising to see _EXPORT macros in a .cc file. > > Each clang, gcc and cl.exe behave differently around the symbol visibility and explicit template instantiation... > I don't know what the best practice is. I /think/ you should only need this on the declaration. But I'm not 100% sure. https://codereview.chromium.org/1699773002/diff/200001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1699773002/diff/200001/base/callback_internal... base/callback_internal.h:113: CallbackBase(BindStateBase* bind_state) Make this one explicit too.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1699773002/diff/200001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1699773002/diff/200001/base/callback_internal... base/callback_internal.h:113: CallbackBase(BindStateBase* bind_state) On 2016/03/02 17:04:56, dcheng wrote: > Make this one explicit too. Done.
lgtm https://codereview.chromium.org/1699773002/diff/220001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/1699773002/diff/220001/base/callback_internal... base/callback_internal.cc:65: CallbackBase<CopyMode::Copyable>::CallbackBase( Same nit from earlier: the definitions should match the order of declarations in the header.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/240001
Updated. Nico: Could you PTAL to this? https://codereview.chromium.org/1699773002/diff/220001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/1699773002/diff/220001/base/callback_internal... base/callback_internal.cc:65: CallbackBase<CopyMode::Copyable>::CallbackBase( On 2016/03/03 21:20:03, dcheng wrote: > Same nit from earlier: the definitions should match the order of declarations in > the header. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks! Also thanks to dcheng for helping with the review, this is really useful. Looking forward to seeing what the next patch in the series is :-) https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/1699773002/diff/180001/base/callback_internal... base/callback_internal.cc:21: BASE_EXPORT CallbackBase<CopyMode::MoveOnly>::CallbackBase(CallbackBase&& c) On 2016/03/02 17:04:56, dcheng wrote: > On 2016/03/02 at 13:09:52, tzik wrote: > > On 2016/03/02 08:02:36, dcheng wrote: > > > Huh. It's surprising to see _EXPORT macros in a .cc file. > > > > Each clang, gcc and cl.exe behave differently around the symbol visibility and > explicit template instantiation... > > I don't know what the best practice is. > > I /think/ you should only need this on the declaration. But I'm not 100% sure. See also ipc/export_template.h for some text (and horrible hacks) about exported templates. https://codereview.chromium.org/1699773002/diff/240001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1699773002/diff/240001/base/callback_internal... base/callback_internal.h:119: extern template class CallbackBase<CopyMode::Copyable>; extern templates tend to always cause lots of problems. Hopefully these will behave better – if so, maybe we can use these as model going forward.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1699773002/#ps260001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699773002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699773002/260001
Message was sent while issue was closed.
Description was changed from ========== Extend base::Callback to have a move-only variant Add CopyMode flag to base::Callback to split it to the copyable variant and the move-only variant. BUG=554299 ========== to ========== Extend base::Callback to have a move-only variant Add CopyMode flag to base::Callback to split it to the copyable variant and the move-only variant. BUG=554299 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Extend base::Callback to have a move-only variant Add CopyMode flag to base::Callback to split it to the copyable variant and the move-only variant. BUG=554299 ========== to ========== Extend base::Callback to have a move-only variant Add CopyMode flag to base::Callback to split it to the copyable variant and the move-only variant. BUG=554299 Committed: https://crrev.com/77d41139d261342a429d2775c59d8e8a386d4c81 Cr-Commit-Position: refs/heads/master@{#380108} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/77d41139d261342a429d2775c59d8e8a386d4c81 Cr-Commit-Position: refs/heads/master@{#380108} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
