Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2042223002:
Introduce OnceClosure and BindOnce (Closed)
DescriptionIntroduce OnceClosure and BindOnce
This CL adds another flag, RepeatMode, to base::Callback to make a Callback
variant that can be run only once, and exposes it as OnceCallback.
This also adds a base:::Bind variant to create an OneOnce, and
adds provides conversions and constraints between Callbacks.
BUG=554299
Committed: https://crrev.com/27d1e313968955f1a120b65b31e316263365b1b3
Cr-Commit-Position: refs/heads/master@{#418177}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : fix #Patch Set 4 : #Patch Set 5 : fix death test #Patch Set 6 : fix #Patch Set 7 : y #Patch Set 8 : rebase #
Total comments: 3
Patch Set 9 : +extract comment on callback.h into docs/callback.md #Patch Set 10 : update #Patch Set 11 : fix #Patch Set 12 : move new variants into internal namespace #Patch Set 13 : doc #Patch Set 14 : rebase #Patch Set 15 : rebase #
Total comments: 24
Patch Set 16 : update docs #
Total comments: 9
Patch Set 17 : attempt to clean up IsCallbackConvertible #Patch Set 18 : +comment #Patch Set 19 : rebase #Patch Set 20 : rebase #Patch Set 21 : update callback.md for Passed and movable types #
Total comments: 19
Patch Set 22 : s/OneShot/Once/g #Patch Set 23 : s/EXPECT_{TRUE,FALSE}/static_assert/. +comments. simplify IsConvertibleCallbacks #
Total comments: 4
Patch Set 24 : split docs changes to a separate CL #Patch Set 25 : rebase #Patch Set 26 : fix indentation #Patch Set 27 : split non-OnceCallback part to a separate CL #Patch Set 28 : fix comment #Patch Set 29 : rebase #
Total comments: 4
Patch Set 30 : swap Once & Repeating positions #
Messages
Total messages: 67 (45 generated)
Description was changed from ========== Combined CL for Callback refinement changes BUG= ========== to ========== Introduce OneShotClosure and BindOneShot BUG=554299 ==========
Description was changed from ========== Introduce OneShotClosure and BindOneShot BUG=554299 ========== to ========== Introduce OneShotClosure and BindOneShot This CL adds another flag, RepeatMode, to base::Callback to make a Callback variant that can be run only once, and exposes it as OneShotCallback. This also adds a base:::Bind variant to create an OneShotCallback, and adds provides conversions and constraints between Callbacks. BUG=554299 ==========
tzik@chromium.org changed reviewers: + dcheng@chromium.org, hiroshige@chromium.org, yutak@chromium.org
PTAL
https://codereview.chromium.org/2042223002/diff/140001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/2042223002/diff/140001/base/bind.h#newcode29 base/bind.h:29: inline base::Callback<MakeUnboundRunType<Functor, Args...>> It might be unclear for developers which Bind()/Callback to use, especially because Callback and RepeatingCallback are the same class. IIUC Bind()/Callback are legacy/unannotated, and new CLs should use BindRepeating() or BindOneShot(). We have two names of Bind()/BindRepeating() (and Callback/RepeatingCallback) of the same type, just for annotating purposes. I think we need comments (and/or PSA?).
https://codereview.chromium.org/2042223002/diff/140001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/2042223002/diff/140001/base/bind.h#newcode29 base/bind.h:29: inline base::Callback<MakeUnboundRunType<Functor, Args...>> On 2016/07/04 10:04:22, hiroshige wrote: > It might be unclear for developers which Bind()/Callback to use, especially > because Callback and RepeatingCallback are the same class. > > IIUC Bind()/Callback are legacy/unannotated, and new CLs should use > BindRepeating() or BindOneShot(). > We have two names of Bind()/BindRepeating() (and Callback/RepeatingCallback) of > the same type, just for annotating purposes. > I think we need comments (and/or PSA?). Hmm, I think it's still not ready to use for devs, since other threading primitives uses old Callbacks. Let me hide new variants into `internal` namespace. I'd propose to unveil them with another wip CL that updates TaskRunners: https://codereview.chromium.org/2122543002/
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2042223002/diff/140001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/2042223002/diff/140001/base/bind.h#newcode29 base/bind.h:29: inline base::Callback<MakeUnboundRunType<Functor, Args...>> On 2016/07/06 07:36:13, tzik wrote: > On 2016/07/04 10:04:22, hiroshige wrote: > > It might be unclear for developers which Bind()/Callback to use, especially > > because Callback and RepeatingCallback are the same class. > > > > IIUC Bind()/Callback are legacy/unannotated, and new CLs should use > > BindRepeating() or BindOneShot(). > > We have two names of Bind()/BindRepeating() (and Callback/RepeatingCallback) > of > > the same type, just for annotating purposes. > > I think we need comments (and/or PSA?). > > Hmm, I think it's still not ready to use for devs, since other threading > primitives uses old Callbacks. Let me hide new variants into `internal` > namespace. > I'd propose to unveil them with another wip CL that updates TaskRunners: > https://codereview.chromium.org/2122543002/ Placing new variants in internal namespace looks good to me.
On 2016/08/12 06:00:47, hiroshige wrote: > https://codereview.chromium.org/2042223002/diff/140001/base/bind.h > File base/bind.h (right): > > https://codereview.chromium.org/2042223002/diff/140001/base/bind.h#newcode29 > base/bind.h:29: inline base::Callback<MakeUnboundRunType<Functor, Args...>> > On 2016/07/06 07:36:13, tzik wrote: > > On 2016/07/04 10:04:22, hiroshige wrote: > > > It might be unclear for developers which Bind()/Callback to use, especially > > > because Callback and RepeatingCallback are the same class. > > > > > > IIUC Bind()/Callback are legacy/unannotated, and new CLs should use > > > BindRepeating() or BindOneShot(). > > > We have two names of Bind()/BindRepeating() (and Callback/RepeatingCallback) > > of > > > the same type, just for annotating purposes. > > > I think we need comments (and/or PSA?). > > > > Hmm, I think it's still not ready to use for devs, since other threading > > primitives uses old Callbacks. Let me hide new variants into `internal` > > namespace. > > I'd propose to unveil them with another wip CL that updates TaskRunners: > > https://codereview.chromium.org/2122543002/ > > Placing new variants in internal namespace looks good to me. Ok, so. hiroshige, yutak, dcheng: PTAL to this?
Just looked through the documentation. Generally it's pretty well written. I'll look into the code changes later. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:26: once. It can handle move-only type better as its bound parameter, and has The sentence after "its bound parameter" looks chopped off? nit: move-only type -> move-only types https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:27: clearer lifetime. Thus, it's recommented to use for a thread hop or result recommen*d*ed Or rather replace the sentence after "Thus, " with: thread hopping or result handling of an asynchronous operation is a good fit for it. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:31: a loose varint. Its internal storage is ref-counted and `RepeatingCallback<>` vari*a*nt https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:34: a thread hop, since it's unpredictable on which thread the callback object is it's unpredictable on -> you cannot predict https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:37: `RepeatingCallback<>` is convertible to `OneShotCallback<>`. By static_cast or other means? https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:48: ## Quick reference for basic stuff stuff -> usage https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:75: void PrintBye() { LOG(INFO) << "bye."; } This looks unused. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:93: `RepeatingCallback<>` can run directly. run -> be run ("run" is intransitive, so "X can run" sounds more like "X is capable of running" than "X is allowed to run".) https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:176: When calling a function bound parameters are first, followed by unbound bound parameters are first -> you must specify bound parameters first (The clause "When calling..." requires the subject "you" in the main sentence.) https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:188: DANGER: weak pointers are not threadsafe, so don't use this nit: **DANGER** to let it stand out. Also you might want to surround the danger sentence with: *** note **DANGER**: Weak pointers are... *** so it stands out more. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:212: Also, smart pointers (e.g. `std::unique_pointer<>`) are supported as the pointer -> ptr https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:258: Ownership of the parameter will be with the callback until the it is run, the it -> it https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:298: Normally parameters are copied in the closure. DANGER: `ConstRef` stores a Ditto regarding DANGER
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:27: clearer lifetime. Thus, it's recommented to use for a thread hop or result On 2016/08/16 03:02:56, Yuta Kitamura wrote: > recommen*d*ed > > Or rather replace the sentence after "Thus, " with: > > thread hopping or result handling of an asynchronous operation is > a good fit for it. Done. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:31: a loose varint. Its internal storage is ref-counted and `RepeatingCallback<>` On 2016/08/16 03:02:55, Yuta Kitamura wrote: > vari*a*nt Done. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:34: a thread hop, since it's unpredictable on which thread the callback object is On 2016/08/16 03:02:56, Yuta Kitamura wrote: > it's unpredictable on -> you cannot predict Done. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:37: `RepeatingCallback<>` is convertible to `OneShotCallback<>`. On 2016/08/16 03:02:55, Yuta Kitamura wrote: > By static_cast or other means? By the implicit conversion, added a comment here. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:48: ## Quick reference for basic stuff On 2016/08/16 03:02:55, Yuta Kitamura wrote: > stuff -> usage Done. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:75: void PrintBye() { LOG(INFO) << "bye."; } On 2016/08/16 03:02:55, Yuta Kitamura wrote: > This looks unused. Done. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:93: `RepeatingCallback<>` can run directly. On 2016/08/16 03:02:56, Yuta Kitamura wrote: > run -> be run > > ("run" is intransitive, so "X can run" sounds more like "X is capable of > running" than "X is allowed to run".) Done. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:176: When calling a function bound parameters are first, followed by unbound On 2016/08/16 03:02:55, Yuta Kitamura wrote: > bound parameters are first -> you must specify bound parameters first > > (The clause "When calling..." requires the subject "you" in the main > sentence.) Hmm, this sentence looks redundant to me. Let me remove this. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:188: DANGER: weak pointers are not threadsafe, so don't use this On 2016/08/16 03:02:56, Yuta Kitamura wrote: > nit: **DANGER** to let it stand out. > > Also you might want to surround the danger sentence with: > > *** note > **DANGER**: Weak pointers are... > *** > > so it stands out more. Done. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:212: Also, smart pointers (e.g. `std::unique_pointer<>`) are supported as the On 2016/08/16 03:02:56, Yuta Kitamura wrote: > pointer -> ptr Done. https://codereview.chromium.org/2042223002/diff/280001/docs/callback.md#newco... docs/callback.md:258: Ownership of the parameter will be with the callback until the it is run, On 2016/08/16 03:02:56, Yuta Kitamura wrote: > the it -> it Done.
LGTM, didn't find anything bad in the change. More eyes would be necessary, though. https://codereview.chromium.org/2042223002/diff/300001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/2042223002/diff/300001/base/bind.h#newcode58 base/bind.h:58: // Unannotated Bind. You probably want to mention that this version is discouraged. https://codereview.chromium.org/2042223002/diff/300001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2042223002/diff/300001/base/callback.h#newcode82 base/callback.h:82: (static_cast<int>(repeat_mode) <= static_cast<int>(other_repeat_mode))>; 1. I feel like this should be called EnableIfConvertibleTo<c, r> because that sounds like a natural way of reading English. 2. Just curious, is it possible to do the following? template <...> using EnableIfConvertibleFrom = typename std::enable_if< ...>::type; This would cut down the extra "typename" and "::type" in the call sites. I'm pretty sure this works on Clang, but not sure about MSVC. https://codereview.chromium.org/2042223002/diff/300001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/2042223002/diff/300001/base/callback_internal... base/callback_internal.cc:35: } Is there any reason why these can't be defined with "= default"?
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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
https://codereview.chromium.org/2042223002/diff/300001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/2042223002/diff/300001/base/bind.h#newcode58 base/bind.h:58: // Unannotated Bind. On 2016/08/16 07:44:12, Yuta Kitamura wrote: > You probably want to mention that this version is discouraged. Done. https://codereview.chromium.org/2042223002/diff/300001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2042223002/diff/300001/base/callback.h#newcode82 base/callback.h:82: (static_cast<int>(repeat_mode) <= static_cast<int>(other_repeat_mode))>; On 2016/08/16 07:44:12, Yuta Kitamura wrote: > 1. I feel like this should be called EnableIfConvertibleTo<c, r> > because that sounds like a natural way of reading English. Hmm, this determines the conversion from CB<c, r> to |this| is available... I feel "ConvertibleTo" is opposite direction. Can I move this part out of class? It will hopefully look more readable. > > 2. Just curious, is it possible to do the following? > > template <...> > using EnableIfConvertibleFrom = typename std::enable_if< > ...>::type; > > This would cut down the extra "typename" and "::type" in the call sites. > > I'm pretty sure this works on Clang, but not sure about MSVC. This doesn't work on MSVC, unfortunately. https://codereview.chromium.org/2042223002/diff/300001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/2042223002/diff/300001/base/callback_internal... base/callback_internal.cc:35: } On 2016/08/16 07:44:12, Yuta Kitamura wrote: > Is there any reason why these can't be defined with "= default"? "= default" is not available for them, since they are conversions from copyable version rather than copy ctor/assign.
Dry run: 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
Dry run: This issue passed the CQ dry run.
(still LGTM++) https://codereview.chromium.org/2042223002/diff/300001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2042223002/diff/300001/base/callback.h#newcode82 base/callback.h:82: (static_cast<int>(repeat_mode) <= static_cast<int>(other_repeat_mode))>; On 2016/08/16 15:19:00, tzik wrote: > Hmm, this determines the conversion from CB<c, r> to |this| is available... > I feel "ConvertibleTo" is opposite direction. > Can I move this part out of class? It will hopefully look more readable. I don't feel strongly on this, but my mental model is: ConvertibleFrom<c, r> <==> this callback is convertible from another with <c, r> <==> another<c,r> converts to this callback ConvertibleTo<c, r> <==> this callback is convertible to another with <c, r> <==> this callback converts to another<c, r> Am I misunderstanding? Or maybe the comment in ll. 75--76 is wrong? https://codereview.chromium.org/2042223002/diff/300001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/2042223002/diff/300001/base/callback_internal... base/callback_internal.cc:35: } On 2016/08/16 15:19:00, tzik wrote: > On 2016/08/16 07:44:12, Yuta Kitamura wrote: > > Is there any reason why these can't be defined with "= default"? > > "= default" is not available for them, since they are conversions from copyable > version rather than copy ctor/assign. Ah okay. I was dumb.
https://codereview.chromium.org/2042223002/diff/300001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2042223002/diff/300001/base/callback.h#newcode82 base/callback.h:82: (static_cast<int>(repeat_mode) <= static_cast<int>(other_repeat_mode))>; On 2016/08/17 05:18:07, Yuta Kitamura wrote: > On 2016/08/16 15:19:00, tzik wrote: > > Hmm, this determines the conversion from CB<c, r> to |this| is available... > > I feel "ConvertibleTo" is opposite direction. > > Can I move this part out of class? It will hopefully look more readable. > > I don't feel strongly on this, but my mental model is: > > ConvertibleFrom<c, r> > <==> this callback is convertible from another with <c, r> > <==> another<c,r> converts to this callback > > ConvertibleTo<c, r> > <==> this callback is convertible to another with <c, r> > <==> this callback converts to another<c, r> > > Am I misunderstanding? Or maybe the comment in ll. 75--76 is wrong? ...! The comment at l.75 was wrong actually. |type| should be defined when |*this| is convertible FROM another.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
tzik@chromium.org changed reviewers: + vabr@chromium.org
LGTM (although I mostly just believed the template magic I saw :)). Cheers, Vaclav https://codereview.chromium.org/2042223002/diff/400001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2042223002/diff/400001/docs/callback.md#newco... docs/callback.md:59: LOG(INFO) << std::move(func_cb2).Run(); // Prints 5. func_cb2 is neither stored nor passed here (it is the result of Run() what is passed), so why the std::move? https://codereview.chromium.org/2042223002/diff/400001/docs/callback.md#newco... docs/callback.md:432: `Owned()` and `RetainedRef()` let `BindState<>` own the exclusive or shared optional nit: "own the exclusive or shared ownership" -> "have the exclusive or shared ownership"
Overall, looks reasonable. Just a few small thoughts to try to help me understand. Also, what do you think of shortening the name of OneShotCallback to OnceCallback? I know it's just a temporary name, but danakj@ and I both feel that if we can shorten this name, even somewhat, that would be better. https://codereview.chromium.org/2042223002/diff/400001/base/bind_unittest.cc File base/bind_unittest.cc (right): https://codereview.chromium.org/2042223002/diff/400001/base/bind_unittest.cc#... base/bind_unittest.cc:1081: EXPECT_TRUE((std::is_constructible< Maybe these should be static_assert? Not sure if we have a convention for that, but it makes more sense to do these at compile time anyway. https://codereview.chromium.org/2042223002/diff/400001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2042223002/diff/400001/base/callback.h#newcode20 base/callback.h:20: // RunMixin provides different variant of `Run()` function to `Callback<>` based Out of curiosity, why this, rather than trying to conditionally enable a Run() method in Callback itself? https://codereview.chromium.org/2042223002/diff/400001/base/callback.h#newcode25 base/callback.h:25: // Specialization for RepeatingCallback. RunMixin provides Run() method that can I think it's a little unclear that the thing that's a lvalue or rvalue reference is the callback itself. Maybe reword to this: Specialization for RepeatingCallback that provides a Run() method that runs the callback without resetting its internal state. or something. I'm not sure how to word this in a way that's not really redundant (so maybe we can just remove the comment instead?) Similar thought for the other specialization. https://codereview.chromium.org/2042223002/diff/400001/base/callback.h#newcode53 base/callback.h:53: CallbackType cb = std::move(*static_cast<CallbackType*>(this)); I'm not sure if it's better or worse, but this could also be written as: static_cast<CallbackType&&>(*this) right? Which would be slightly shorter and possibly more readable... I'm not 100% sure. Also, let's add a comment that it's important to do it by moving it into a local variable rather than resetting after invoking the captured function, since the latter is not safe (dispatching the function may destroy |this|) https://codereview.chromium.org/2042223002/diff/400001/base/callback.h#newcode65 base/callback.h:65: template <typename From, typename To, Is it possible to partially specialize this to ensure it only matches on specializations of base::Callback? Or enforce some other way? https://codereview.chromium.org/2042223002/diff/400001/base/callback.h#newcode96 base/callback.h:96: "OneShot Callback must be MoveOnly."); Nit: OneShotCallback https://codereview.chromium.org/2042223002/diff/400001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/2042223002/diff/400001/base/callback_internal... base/callback_internal.cc:28: : bind_state_(c.bind_state_), polymorphic_invoke_(c.polymorphic_invoke_) {} If we're creating it from a copyable callback, should we require moving state out of the original copyable callback? Otherwise, we clone the state, which defeats the point of a move-only callback, right?
Description was changed from ========== Introduce OneShotClosure and BindOneShot This CL adds another flag, RepeatMode, to base::Callback to make a Callback variant that can be run only once, and exposes it as OneShotCallback. This also adds a base:::Bind variant to create an OneShotCallback, and adds provides conversions and constraints between Callbacks. BUG=554299 ========== to ========== Introduce OnceClosure and BindOnce This CL adds another flag, RepeatMode, to base::Callback to make a Callback variant that can be run only once, and exposes it as OnceCallback. This also adds a base:::Bind variant to create an OneOnce, and adds provides conversions and constraints between Callbacks. BUG=554299 ==========
https://codereview.chromium.org/2042223002/diff/400001/base/bind_unittest.cc File base/bind_unittest.cc (right): https://codereview.chromium.org/2042223002/diff/400001/base/bind_unittest.cc#... base/bind_unittest.cc:1081: EXPECT_TRUE((std::is_constructible< On 2016/08/26 07:37:56, dcheng wrote: > Maybe these should be static_assert? Not sure if we have a convention for that, > but it makes more sense to do these at compile time anyway. Done. https://codereview.chromium.org/2042223002/diff/400001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2042223002/diff/400001/base/callback.h#newcode20 base/callback.h:20: // RunMixin provides different variant of `Run()` function to `Callback<>` based On 2016/08/26 07:37:56, dcheng wrote: > Out of curiosity, why this, rather than trying to conditionally enable a Run() > method in Callback itself? This is to avoid using template for on Callback<>::Run. Some of its users take the address of Callback<>::Run and call it later, but it's not compatible to the template. https://codereview.chromium.org/2042223002/diff/400001/base/callback.h#newcode25 base/callback.h:25: // Specialization for RepeatingCallback. RunMixin provides Run() method that can On 2016/08/26 07:37:57, dcheng wrote: > I think it's a little unclear that the thing that's a lvalue or rvalue reference > is the callback itself. > > Maybe reword to this: > Specialization for RepeatingCallback that provides a Run() method that runs the > callback without resetting its internal state. > > or something. I'm not sure how to word this in a way that's not really redundant > (so maybe we can just remove the comment instead?) > > Similar thought for the other specialization. Hmm, that doesn't add much information. Let me just remove the second sentence. https://codereview.chromium.org/2042223002/diff/400001/base/callback.h#newcode53 base/callback.h:53: CallbackType cb = std::move(*static_cast<CallbackType*>(this)); On 2016/08/26 07:37:56, dcheng wrote: > I'm not sure if it's better or worse, but this could also be written as: > > static_cast<CallbackType&&>(*this) > > right? Which would be slightly shorter and possibly more readable... I'm not > 100% sure. > > Also, let's add a comment that it's important to do it by moving it into a local > variable rather than resetting after invoking the captured function, since the > latter is not safe (dispatching the function may destroy |this|) Done. https://codereview.chromium.org/2042223002/diff/400001/base/callback.h#newcode65 base/callback.h:65: template <typename From, typename To, On 2016/08/26 07:37:56, dcheng wrote: > Is it possible to partially specialize this to ensure it only matches on > specializations of base::Callback? Or enforce some other way? Done. Yes, partial specialization looks better here. It was originally planned to cover general cases, but there's only one case now. https://codereview.chromium.org/2042223002/diff/400001/base/callback.h#newcode96 base/callback.h:96: "OneShot Callback must be MoveOnly."); On 2016/08/26 07:37:57, dcheng wrote: > Nit: OneShotCallback Done. https://codereview.chromium.org/2042223002/diff/400001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2042223002/diff/400001/docs/callback.md#newco... docs/callback.md:59: LOG(INFO) << std::move(func_cb2).Run(); // Prints 5. On 2016/08/24 11:44:06, vabr (Chromium) wrote: > func_cb2 is neither stored nor passed here (it is the result of Run() what is > passed), so why the std::move? Since func_cb2 is a variant of Callback that can be called only once, we are implementing its Run() as a rvalue-reference-only function. That implies that we need std::move() here, so that the caller passes |func_cb2| into Run() itself. https://codereview.chromium.org/2042223002/diff/400001/docs/callback.md#newco... docs/callback.md:432: `Owned()` and `RetainedRef()` let `BindState<>` own the exclusive or shared On 2016/08/24 11:44:06, vabr (Chromium) wrote: > optional nit: "own the exclusive or shared ownership" -> "have the exclusive or > shared ownership" Done.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2042223002/diff/400001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/2042223002/diff/400001/base/callback_internal... base/callback_internal.cc:28: : bind_state_(c.bind_state_), polymorphic_invoke_(c.polymorphic_invoke_) {} On 2016/08/26 07:37:57, dcheng wrote: > If we're creating it from a copyable callback, should we require moving state > out of the original copyable callback? Otherwise, we clone the state, which > defeats the point of a move-only callback, right? Any thoughts on this comment? https://codereview.chromium.org/2042223002/diff/440001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2042223002/diff/440001/base/callback.h#newcode53 base/callback.h:53: // It's not safe to touch |this| after the invocation, since it may destroy Nit: since it => since running the bound function https://codereview.chromium.org/2042223002/diff/440001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2042223002/diff/440001/docs/callback.md#newcode1 docs/callback.md:1: # base::Callback<> and base::Bind() Btw, can we move this in a separate CL? That way, it'll make the documentation changes easier to diff as well (which might help future code research as well).
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/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
https://codereview.chromium.org/2042223002/diff/400001/base/callback_internal.cc File base/callback_internal.cc (right): https://codereview.chromium.org/2042223002/diff/400001/base/callback_internal... base/callback_internal.cc:28: : bind_state_(c.bind_state_), polymorphic_invoke_(c.polymorphic_invoke_) {} On 2016/08/31 06:20:53, dcheng wrote: > On 2016/08/26 07:37:57, dcheng wrote: > > If we're creating it from a copyable callback, should we require moving state > > out of the original copyable callback? Otherwise, we clone the state, which > > defeats the point of a move-only callback, right? > > Any thoughts on this comment? Oh, sorry. I missed this comment. Since the original callback is copyable, the caller can keep a copy of it anyway. So I think it's not critical to keep the move-onliness. https://codereview.chromium.org/2042223002/diff/440001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2042223002/diff/440001/base/callback.h#newcode53 base/callback.h:53: // It's not safe to touch |this| after the invocation, since it may destroy On 2016/08/31 06:20:53, dcheng wrote: > Nit: since it => since running the bound function Done. https://codereview.chromium.org/2042223002/diff/440001/docs/callback.md File docs/callback.md (right): https://codereview.chromium.org/2042223002/diff/440001/docs/callback.md#newcode1 docs/callback.md:1: # base::Callback<> and base::Bind() On 2016/08/31 06:20:53, dcheng wrote: > Btw, can we move this in a separate CL? That way, it'll make the documentation > changes easier to diff as well (which might help future code research as well). Sounds good. Done.
Dry run: 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
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM https://codereview.chromium.org/2042223002/diff/560001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2042223002/diff/560001/base/callback.h#newcode24 base/callback.h:24: // RunMixin provides different variant of `Run()` function to `Callback<>` based Nit: "different variants" or "a different variant" https://codereview.chromium.org/2042223002/diff/560001/base/callback_forward.h File base/callback_forward.h (right): https://codereview.chromium.org/2042223002/diff/560001/base/callback_forward.... base/callback_forward.h:19: Once, Super minor optional nit: here, the order is Once, Repeating, but everywhere else is Repeating, Once. I think it would be nice to be uniform (my preferred order is Once, Repeating since we want to guide people to use OnceCallback) but that requires moving more things around. Again, this is totally optional and up to you if you want to.
https://codereview.chromium.org/2042223002/diff/560001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2042223002/diff/560001/base/callback.h#newcode24 base/callback.h:24: // RunMixin provides different variant of `Run()` function to `Callback<>` based On 2016/09/08 04:09:01, dcheng wrote: > Nit: "different variants" or "a different variant" Done. https://codereview.chromium.org/2042223002/diff/560001/base/callback_forward.h File base/callback_forward.h (right): https://codereview.chromium.org/2042223002/diff/560001/base/callback_forward.... base/callback_forward.h:19: Once, On 2016/09/08 04:09:01, dcheng wrote: > Super minor optional nit: here, the order is Once, Repeating, but everywhere > else is Repeating, Once. > > I think it would be nice to be uniform (my preferred order is Once, Repeating > since we want to guide people to use OnceCallback) but that requires moving more > things around. > > Again, this is totally optional and up to you if you want to. Done. I moved all .*Once.* before Repeating.
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/v2/patch-status/codereview.chromium.or...
Let me update the documentation in docs/callback.md as a separate CL before moving new callbacks out of internal namespace.
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 yutak@chromium.org, vabr@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2042223002/#ps580001 (title: "swap Once & Repeating positions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Introduce OnceClosure and BindOnce This CL adds another flag, RepeatMode, to base::Callback to make a Callback variant that can be run only once, and exposes it as OnceCallback. This also adds a base:::Bind variant to create an OneOnce, and adds provides conversions and constraints between Callbacks. BUG=554299 ========== to ========== Introduce OnceClosure and BindOnce This CL adds another flag, RepeatMode, to base::Callback to make a Callback variant that can be run only once, and exposes it as OnceCallback. This also adds a base:::Bind variant to create an OneOnce, and adds provides conversions and constraints between Callbacks. BUG=554299 ==========
Message was sent while issue was closed.
Committed patchset #30 (id:580001)
Message was sent while issue was closed.
Description was changed from ========== Introduce OnceClosure and BindOnce This CL adds another flag, RepeatMode, to base::Callback to make a Callback variant that can be run only once, and exposes it as OnceCallback. This also adds a base:::Bind variant to create an OneOnce, and adds provides conversions and constraints between Callbacks. BUG=554299 ========== to ========== Introduce OnceClosure and BindOnce This CL adds another flag, RepeatMode, to base::Callback to make a Callback variant that can be run only once, and exposes it as OnceCallback. This also adds a base:::Bind variant to create an OneOnce, and adds provides conversions and constraints between Callbacks. BUG=554299 Committed: https://crrev.com/27d1e313968955f1a120b65b31e316263365b1b3 Cr-Commit-Position: refs/heads/master@{#418177} ==========
Message was sent while issue was closed.
Patchset 30 (id:??) landed as https://crrev.com/27d1e313968955f1a120b65b31e316263365b1b3 Cr-Commit-Position: refs/heads/master@{#418177} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
