Do Perfect Forwarding from base::Bind to BindState storage

Do Perfect Forwarding from base::Bind to BindState storage

Replace StorageType usage with std::decay and do Perfect Forwarding from
base::Bind to BindState storage as a preparation to store a move-only
type into BindState.

The difference of StorageType and std::decay is essentially array
handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
OTOH std::decay converts it to "T*".

BUG=554299
Committed: https://crrev.com/cdec75e36dd5ec9c5643f7b473e166a3acef81ea
Cr-Commit-Position: refs/heads/master@{#372900}

[tzik, 2016-01-28 08:32:00]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-28 08:33:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644603003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644603003/1

[commit-bot: I haz the power, 2016-01-28 10:18:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-28 10:18:25]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-01-28 12:43:27]

tzik@chromium.org changed reviewers:
+ danakj@chromium.org

[tzik, 2016-01-28 12:43:28]

PTAL

[tzik, 2016-01-28 13:15:19]

Description was changed from

==========
Remove CallbackParamTraits::StorageType

Replace StorageType usage with std::decay and do Perfect Forwarding from
base::Bind to base::BindState.

The difference of StorageType and std::decay is essentially array
handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
OTOH std::decay converts it to "T*".
==========

to

==========
Remove CallbackParamTraits::StorageType

Replace StorageType usage with std::decay and do Perfect Forwarding from
base::Bind to base::BindState.

The difference of StorageType and std::decay is essentially array
handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
OTOH std::decay converts it to "T*".

BUG=561749
==========

[danakj, 2016-01-28 23:01:00]

Can you add some motivation to the CL description about why you want to make
this change?

I feel like your goal is to get rid of CallbackParamTraits entirely, and do
proper forwarding as much as possible? But would be nice to explain on the CL,
and maybe I'm wrong!

https://codereview.chromium.org/1644603003/diff/1/base/bind.h
File base/bind.h (right):

https://codereview.chromium.org/1644603003/diff/1/base/bind.h#newcode9
base/bind.h:9: #include "base/callback_internal.h"
Should this be removed? (bind_internal.h still uses and includes it, but this
file isn't using anything directly from there now is it?)

https://codereview.chromium.org/1644603003/diff/1/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/1644603003/diff/1/base/bind_internal.h#newcod...
base/bind_internal.h:406: BindState(const Runnable& runnable, ForwardArgs&&...
bound_args)
Just as a quick sanity check.. does this have a visible effect on binary size?

https://codereview.chromium.org/1644603003/diff/1/base/callback_internal.h
File base/callback_internal.h (left):

https://codereview.chromium.org/1644603003/diff/1/base/callback_internal.h#ol...
base/callback_internal.h:132: // extract a suitable type for storing and
forwarding arguments.
Comment is out of date now

https://codereview.chromium.org/1644603003/diff/1/base/callback_internal.h#ol...
base/callback_internal.h:135: // pointers for storage; and it avoids
accidentally trying to create a
And here

https://codereview.chromium.org/1644603003/diff/1/base/callback_internal.h#ol...
base/callback_internal.h:161: struct CallbackParamTraitsForNonMoveOnlyType<T&> {
Do we still need this specialization?

[tzik, 2016-02-01 14:39:31]

Description was changed from

==========
Remove CallbackParamTraits::StorageType

Replace StorageType usage with std::decay and do Perfect Forwarding from
base::Bind to base::BindState.

The difference of StorageType and std::decay is essentially array
handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
OTOH std::decay converts it to "T*".

BUG=561749
==========

to

==========
Do Perfect Forwarding from base::Bind to BindState storage

Replace StorageType usage with std::decay and do Perfect Forwarding from
base::Bind to BindState storage as a preparation to store a move-only
type into BindState.

The difference of StorageType and std::decay is essentially array
handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
OTOH std::decay converts it to "T*".

BUG=561749
==========

[tzik, 2016-02-01 14:59:42]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[tzik, 2016-02-01 15:00:09]

On 2016/01/28 23:01:00, danakj wrote:
> Can you add some motivation to the CL description about why you want to make
> this change?
> 
> I feel like your goal is to get rid of CallbackParamTraits entirely, and do
> proper forwarding as much as possible? But would be nice to explain on the CL,
> and maybe I'm wrong!
> 

Updated the CL description. Let me shift the focus on the description to
"forwarding".

Removing CallbackParamTraits is one of the goal, but I'm not sure we can do it
soon.
At the first milestone, I want base::Bind to be able to bind move-only types,
including move-only Runnables.
And for it, we need to forward arguments by Universal Reference rather than
const ref.

[tzik, 2016-02-01 15:00:16]

https://codereview.chromium.org/1644603003/diff/1/base/bind.h
File base/bind.h (right):

https://codereview.chromium.org/1644603003/diff/1/base/bind.h#newcode9
base/bind.h:9: #include "base/callback_internal.h"
On 2016/01/28 23:01:00, danakj wrote:
> Should this be removed? (bind_internal.h still uses and includes it, but this
> file isn't using anything directly from there now is it?)

Done.

https://codereview.chromium.org/1644603003/diff/1/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/1644603003/diff/1/base/bind_internal.h#newcod...
base/bind_internal.h:406: BindState(const Runnable& runnable, ForwardArgs&&...
bound_args)
On 2016/01/28 23:01:00, danakj wrote:
> Just as a quick sanity check.. does this have a visible effect on binary size?

Good point. Yes, this increases the stripped binary size by 8kB on Linux.
This tends to be inlined into the caller...

https://codereview.chromium.org/1644603003/diff/1/base/callback_internal.h
File base/callback_internal.h (left):

https://codereview.chromium.org/1644603003/diff/1/base/callback_internal.h#ol...
base/callback_internal.h:132: // extract a suitable type for storing and
forwarding arguments.
On 2016/01/28 23:01:00, danakj wrote:
> Comment is out of date now

Done.

https://codereview.chromium.org/1644603003/diff/1/base/callback_internal.h#ol...
base/callback_internal.h:135: // pointers for storage; and it avoids
accidentally trying to create a
On 2016/01/28 23:01:00, danakj wrote:
> And here

Done.

https://codereview.chromium.org/1644603003/diff/1/base/callback_internal.h#ol...
base/callback_internal.h:161: struct CallbackParamTraitsForNonMoveOnlyType<T&> {
On 2016/01/28 23:01:00, danakj wrote:
> Do we still need this specialization?

Done.

[commit-bot: I haz the power, 2016-02-01 15:03:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644603003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644603003/20001

[tzik, 2016-02-01 15:03:41]

Description was changed from

==========
Do Perfect Forwarding from base::Bind to BindState storage

Replace StorageType usage with std::decay and do Perfect Forwarding from
base::Bind to BindState storage as a preparation to store a move-only
type into BindState.

The difference of StorageType and std::decay is essentially array
handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
OTOH std::decay converts it to "T*".

BUG=561749
==========

to

==========
Do Perfect Forwarding from base::Bind to BindState storage

Replace StorageType usage with std::decay and do Perfect Forwarding from
base::Bind to BindState storage as a preparation to store a move-only
type into BindState.

The difference of StorageType and std::decay is essentially array
handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
OTOH std::decay converts it to "T*".

BUG=554299
==========

[commit-bot: I haz the power, 2016-02-01 16:34:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-01 16:34:41]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-02-01 19:44:18]

At this point, except for our whitelist, people could bind a scoped_ptr into a
Callback, without base::Passed(), I guess? But then the function would have to
take a scoped_ptr const&. So this would kinda compete with base::Owned if we
removed the whitelist (which we can't without also replacing base::Passed).

So.. all that is to say, I guess this isn't actually making 2 ways to do
anything yet. You either add a type T to the whitelist, and then you'll have to
use Passed/Owned, or you don't and then you can skip them, but the callback will
always own the thing and not pass ownership to the function. I don't really
expect the latter to show up, but if it does I don't think it will be
problematic to us in the future.

So I think that this LGTM.

[Nico, 2016-02-01 21:04:49]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-02-01 21:04:50]

lgtm, looks like a nice change :-)

[tzik, 2016-02-02 05:56:04]

On 2016/02/01 19:44:18, danakj wrote:
> At this point, except for our whitelist, people could bind a scoped_ptr into a
> Callback, without base::Passed(), I guess? But then the function would have to
> take a scoped_ptr const&.

We are almost there, but it needs to migrate from std::tuple to base::Tuple, for
which I'm going to post a proposal to cxx@chromium.org.

> So this would kinda compete with base::Owned if we
> removed the whitelist (which we can't without also replacing base::Passed).
> 
> So.. all that is to say, I guess this isn't actually making 2 ways to do
> anything yet. You either add a type T to the whitelist, and then you'll have
to
> use Passed/Owned, or you don't and then you can skip them, but the callback
will
> always own the thing and not pass ownership to the function. I don't really
> expect the latter to show up, but if it does I don't think it will be
> problematic to us in the future.
> 
> So I think that this LGTM.

[tzik, 2016-02-02 05:56:17]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2016-02-02 05:56:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644603003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644603003/20001

[commit-bot: I haz the power, 2016-02-02 06:09:47]

Description was changed from

==========
Do Perfect Forwarding from base::Bind to BindState storage

Replace StorageType usage with std::decay and do Perfect Forwarding from
base::Bind to BindState storage as a preparation to store a move-only
type into BindState.

The difference of StorageType and std::decay is essentially array
handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
OTOH std::decay converts it to "T*".

BUG=554299
==========

to

==========
Do Perfect Forwarding from base::Bind to BindState storage

Replace StorageType usage with std::decay and do Perfect Forwarding from
base::Bind to BindState storage as a preparation to store a move-only
type into BindState.

The difference of StorageType and std::decay is essentially array
handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
OTOH std::decay converts it to "T*".

BUG=554299
==========

[commit-bot: I haz the power, 2016-02-02 06:09:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-02 06:10:36]

Description was changed from

==========
Do Perfect Forwarding from base::Bind to BindState storage

Replace StorageType usage with std::decay and do Perfect Forwarding from
base::Bind to BindState storage as a preparation to store a move-only
type into BindState.

The difference of StorageType and std::decay is essentially array
handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
OTOH std::decay converts it to "T*".

BUG=554299
==========

to

==========
Do Perfect Forwarding from base::Bind to BindState storage

Replace StorageType usage with std::decay and do Perfect Forwarding from
base::Bind to BindState storage as a preparation to store a move-only
type into BindState.

The difference of StorageType and std::decay is essentially array
handling. CallbackParamTraits::StorageType converts "T[n]" to "const T*"
OTOH std::decay converts it to "T*".

BUG=554299

Committed: https://crrev.com/cdec75e36dd5ec9c5643f7b473e166a3acef81ea
Cr-Commit-Position: refs/heads/master@{#372900}
==========

[commit-bot: I haz the power, 2016-02-02 06:10:37]

Patchset 2 (id:??) landed as
https://crrev.com/cdec75e36dd5ec9c5643f7b473e166a3acef81ea
Cr-Commit-Position: refs/heads/master@{#372900}

[tzik, 2016-02-02 06:57:38]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1654973003/ by tzik@chromium.org.

The reason for reverting is: This CL broke Windows build at an instantiation
for:
e:\b\build\slave\win_x64_gn__dbg_\build\src\components\mus\gles2\command_buffer_local.cc(195)

The error message is:
e:\b\build\slave\win_x64_gn__dbg_\build\src\base\bind_internal.h(291) : warning
C4267: 'argument' : conversion from 'size_t' to 'const uint32_t', possible loss
of data

https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN%20%28dbg%29....

[tzik, 2016-02-02 12:29:11]

On 2016/02/02 05:56:04, tzik wrote:
> On 2016/02/01 19:44:18, danakj wrote:
> > At this point, except for our whitelist, people could bind a scoped_ptr into
a
> > Callback, without base::Passed(), I guess? But then the function would have
to
> > take a scoped_ptr const&.
> 
> We are almost there, but it needs to migrate from std::tuple to base::Tuple,
for
> which I'm going to post a proposal to mailto:cxx@chromium.org.
> 

Ah... std::tuple increases the stripped binary size by 12kB....

[danakj, 2016-02-02 20:18:46]

On Tue, Feb 2, 2016 at 4:29 AM, <tzik@chromium.org> wrote:

> On 2016/02/02 05:56:04, tzik wrote:
> > On 2016/02/01 19:44:18, danakj wrote:
> > > At this point, except for our whitelist, people could bind a scoped_ptr
into
> a
> > > Callback, without base::Passed(), I guess? But then the function would
have
> to
> > > take a scoped_ptr const&.
> >
> > We are almost there, but it needs to migrate from std::tuple to
base::Tuple,
> for
> > which I'm going to post a proposal to mailto:cxx@chromium.org.
> >
>
> Ah... std::tuple increases the stripped binary size by 12kB....
>
>
I asked sievers@ about 8kB on android and he said that would be in the
noise. I guess 12 is also..

But then, why is it doing so? Maybe we could do something more
specific/smaller just for this codesite? How much of the API of tuple do we
need?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.