Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1798403002:
Support rvalue-reference Runnables in base::Bind internals (Closed)
DescriptionSupport rvalue-reference Runnables in base::Bind internals
This is a preparation of base::Bind for move-only variants of Callback,
and also a preparation of the oneshot variant of Callback.
On InvokeHelper mod, this CL moves Runnable from a class template
parameter to a function template parameter of InvokeHelper::MakeItSo,
so that it can run &&-qualified Run() method via rvalue-reference
Runnable.
On BindState mod, BindState ctor will forward Runnable as Perfect
Forwarding into its internal storage. So that BindState can hold
move-only Runnable into it.
BUG=554299
Committed: https://crrev.com/ee2487294417a82adfc854aa680c7765eef7494e
Cr-Commit-Position: refs/heads/master@{#397083}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #
Total comments: 7
Patch Set 4 : s/IsWeakCall/is_weak_call/. Perfect Forwarding -> pass-by-value + std::move() #Messages
Total messages: 33 (14 generated)
Description was changed from ========== description tbd ========== to ========== Do Perfect Forwarding for Runnable in base::Bind impl * Move Runnable from InvokeHelper template argument to its MakeItSo template argument, so that MakeItSo can run rvalue-reference runnables. * Forward Runnable in BindState constructor into its internal storage, so that it can bind move-only runnables. 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/1798403002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1798403002/1
tzik@chromium.org changed reviewers: + dcheng@chromium.org, thakis@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
Is this change intended to help support one-shot callbacks? I guess the Run() method on that will be &&-qualified?
On 2016/03/15 23:04:55, dcheng wrote: > Is this change intended to help support one-shot callbacks? I guess the Run() > method on that will be &&-qualified? Yes, exactly. It's a requirement to run one-shot callbacks, that will have &&-qualified Run().
LGTM, but let's update the CL description to include the direct motivation to support one-shot callbacks that have a &&-qualified Run().
Description was changed from
==========
Do Perfect Forwarding for Runnable in base::Bind impl
* Move Runnable from InvokeHelper template argument to its MakeItSo
template argument, so that MakeItSo can run rvalue-reference
runnables.
* Forward Runnable in BindState constructor into its internal storage,
so that it can bind move-only runnables.
BUG=554299
==========
to
==========
Support rvalue-reference Runnables in base::Bind internals
This is a preparation of base::Bind for move-only variants of Callback,
and also a preparation of the oneshot variant of Callback.
On InvokeHelper mod, this CL moves Runnable from a class template
parameter to a function template parameter of InvokeHelper::MakeItSo,
so that it can run &&-qualified Run() method via rvalue-reference
Runnable.
On BindState mod, BindState ctor will forward Runnable as Perfect
Forwarding into its internal storage. So that BindState can hold
move-only Runnable into it.
BUG=554299
==========
On 2016/03/16 19:52:34, dcheng wrote: > LGTM, but let's update the CL description to include the direct motivation to > support one-shot callbacks that have a &&-qualified Run(). Done!
Nico: could you take a look to this?
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/1798403002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1798403002/20001
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/patch-status/1798403002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1798403002/40001
tzik@chromium.org changed reviewers: + danakj@chromium.org
Dana: could you take a look to this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1798403002/diff/40001/base/bind_internal.h File base/bind_internal.h (right): https://codereview.chromium.org/1798403002/diff/40001/base/bind_internal.h#ne... base/bind_internal.h:296: template <bool IsWeakCall, typename ReturnType> mind making this consistent and make it is_weak_call like you did below? https://codereview.chromium.org/1798403002/diff/40001/base/bind_internal.h#ne... base/bind_internal.h:364: storage->runnable_, Unwrap(get<bound_indices>(storage->bound_args_))..., IIUC we'll make another Run() function in Invoker than is rvalue-qualified and it will move the storage->runnable_ ? https://codereview.chromium.org/1798403002/diff/40001/base/bind_internal.h#ne... base/bind_internal.h:427: runnable_(std::forward<ForwardRunnable>(runnable)), why not take by value and std::move here?
Updated! https://codereview.chromium.org/1798403002/diff/40001/base/bind_internal.h File base/bind_internal.h (right): https://codereview.chromium.org/1798403002/diff/40001/base/bind_internal.h#ne... base/bind_internal.h:296: template <bool IsWeakCall, typename ReturnType> On 2016/05/19 21:18:58, danakj wrote: > mind making this consistent and make it is_weak_call like you did below? Done. https://codereview.chromium.org/1798403002/diff/40001/base/bind_internal.h#ne... base/bind_internal.h:364: storage->runnable_, Unwrap(get<bound_indices>(storage->bound_args_))..., On 2016/05/19 21:18:59, danakj wrote: > IIUC we'll make another Run() function in Invoker than is rvalue-qualified and > it will move the storage->runnable_ ? After we add OneShot variant of Callback, we'll have another variant of Invoker that moves out storage->runnable_, rather than adding new Run() to Invoker, since it will double the binary size of instantiated Invoker::Run(). https://codereview.chromium.org/1798403002/diff/40001/base/bind_internal.h#ne... base/bind_internal.h:427: runnable_(std::forward<ForwardRunnable>(runnable)), On 2016/05/19 21:18:59, danakj wrote: > why not take by value and std::move here? Done.
LGTM https://codereview.chromium.org/1798403002/diff/40001/base/bind_internal.h File base/bind_internal.h (right): https://codereview.chromium.org/1798403002/diff/40001/base/bind_internal.h#ne... base/bind_internal.h:364: storage->runnable_, Unwrap(get<bound_indices>(storage->bound_args_))..., On 2016/05/20 06:53:54, tzik wrote: > On 2016/05/19 21:18:59, danakj wrote: > > IIUC we'll make another Run() function in Invoker than is rvalue-qualified and > > it will move the storage->runnable_ ? > > After we add OneShot variant of Callback, we'll have another variant of Invoker > that moves out storage->runnable_, rather than adding new Run() to Invoker, > since it will double the binary size of instantiated Invoker::Run(). Ah, it would become larger because of function overloading? This is implementation details of the future.. but just wondering would a RunOnce() function on this invoker work as well?
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 Link to the patchset: https://codereview.chromium.org/1798403002/#ps60001 (title: "s/IsWeakCall/is_weak_call/. Perfect Forwarding -> pass-by-value + std::move()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1798403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1798403002/60001
Message was sent while issue was closed.
Description was changed from
==========
Support rvalue-reference Runnables in base::Bind internals
This is a preparation of base::Bind for move-only variants of Callback,
and also a preparation of the oneshot variant of Callback.
On InvokeHelper mod, this CL moves Runnable from a class template
parameter to a function template parameter of InvokeHelper::MakeItSo,
so that it can run &&-qualified Run() method via rvalue-reference
Runnable.
On BindState mod, BindState ctor will forward Runnable as Perfect
Forwarding into its internal storage. So that BindState can hold
move-only Runnable into it.
BUG=554299
==========
to
==========
Support rvalue-reference Runnables in base::Bind internals
This is a preparation of base::Bind for move-only variants of Callback,
and also a preparation of the oneshot variant of Callback.
On InvokeHelper mod, this CL moves Runnable from a class template
parameter to a function template parameter of InvokeHelper::MakeItSo,
so that it can run &&-qualified Run() method via rvalue-reference
Runnable.
On BindState mod, BindState ctor will forward Runnable as Perfect
Forwarding into its internal storage. So that BindState can hold
move-only Runnable into it.
BUG=554299
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
Support rvalue-reference Runnables in base::Bind internals
This is a preparation of base::Bind for move-only variants of Callback,
and also a preparation of the oneshot variant of Callback.
On InvokeHelper mod, this CL moves Runnable from a class template
parameter to a function template parameter of InvokeHelper::MakeItSo,
so that it can run &&-qualified Run() method via rvalue-reference
Runnable.
On BindState mod, BindState ctor will forward Runnable as Perfect
Forwarding into its internal storage. So that BindState can hold
move-only Runnable into it.
BUG=554299
==========
to
==========
Support rvalue-reference Runnables in base::Bind internals
This is a preparation of base::Bind for move-only variants of Callback,
and also a preparation of the oneshot variant of Callback.
On InvokeHelper mod, this CL moves Runnable from a class template
parameter to a function template parameter of InvokeHelper::MakeItSo,
so that it can run &&-qualified Run() method via rvalue-reference
Runnable.
On BindState mod, BindState ctor will forward Runnable as Perfect
Forwarding into its internal storage. So that BindState can hold
move-only Runnable into it.
BUG=554299
Committed: https://crrev.com/ee2487294417a82adfc854aa680c7765eef7494e
Cr-Commit-Position: refs/heads/master@{#397083}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ee2487294417a82adfc854aa680c7765eef7494e Cr-Commit-Position: refs/heads/master@{#397083} |
