Bind: Use VariadicTemplates in bind_internal.h

Bind: Use Variadic Templates in bind_internal.h

* Introduce TypeList as helpers.
* Drop bind_internal.h.pump and replace generated codes with Variadic Templates version.

BUG=433164
Committed: https://crrev.com/8ce65709225bac5922e6b2b80a912cf9796949b1
Cr-Commit-Position: refs/heads/master@{#314847}

[tzik, 2014-12-03 05:23:23]

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

[tzik, 2014-12-03 05:59:08]

PTAL

[Nico, 2014-12-08 19:58:08]

Super cool!

From a high-level point of view, it's strange that there's now base::Tuple and
base::internal::Tuple and they're different. Maybe we can change base/tuple.h to
support what this needs? Do you think that's possible? If so, do you think you
could give that a try?

(See https://codereview.chromium.org/693693005/ for prior art – bind needing
this is probably good motivation for changing tuple, but that CL isn't enough
for what you need anyways.)

https://codereview.chromium.org/743853002/diff/60001/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/743853002/diff/60001/base/bind_helpers.h#newc...
base/bind_helpers.h:483: // P1 should be the type of the object that will be
received of the method.
nit: "P1" in comment doesn't make sense any more now that P1 is gone

https://codereview.chromium.org/743853002/diff/60001/base/bind_helpers.h#newc...
base/bind_helpers.h:491: struct IsWeakMethod<true, ConstRefWrapper<WeakPtr<T> >,
Args...>
nit: remove space between > >

(i think `git cl format` will do this for you)

https://codereview.chromium.org/743853002/diff/60001/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/743853002/diff/60001/base/bind_internal.h#new...
base/bind_internal.h:62: //                    which grows at O(n^2) with the
arity.
I guess this is still true, except that they're now created by the compiler on
demand. So I guess the comment still makes sense?

https://codereview.chromium.org/743853002/diff/60001/base/bind_internal.h#new...
base/bind_internal.h:324: template <typename BoundIndexes,
nit: "index" confusingly has two valid plurals, "indices" and "indexes", and it
depends on the context which one is correct. I think here it refers to an index
in the sense of a subscript, and I believe the right plural in this case is
"indices"

https://codereview.chromium.org/743853002/diff/60001/base/bind_internal.h#new...
base/bind_internal.h:329: template <size_t... bound_indexes,
likewise

https://codereview.chromium.org/743853002/diff/60001/base/callback_tuple.h
File base/callback_tuple.h (right):

https://codereview.chromium.org/743853002/diff/60001/base/callback_tuple.h#ne...
base/callback_tuple.h:58: : NthTypeImpl<n - 1, TypeList<Types...>> {
:-/

(I know that there's no better way)

[tzik, 2014-12-09 09:46:53]

On 2014/12/08 19:58:08, Nico wrote:
> Super cool!
> 
> From a high-level point of view, it's strange that there's now base::Tuple and
> base::internal::Tuple and they're different. Maybe we can change base/tuple.h
to
> support what this needs? Do you think that's possible? If so, do you think you
> could give that a try?
> 
> (See https://codereview.chromium.org/693693005/ for prior art – bind needing
> this is probably good motivation for changing tuple, but that CL isn't enough
> for what you need anyways.)
> 

I believe it's possible.
What I need are uniform construction of the tuple type (like Tuple<int, char,
void*>), index access to a item and its type.
If we add them to base::Tuple, it will work for this CL.
I have yet another CL for tuples: https://codereview.chromium.org/785153002/
# Didn't know mdempsky@ made another, I think his version should also work.

[tzik, 2014-12-09 14:50:27]

https://codereview.chromium.org/743853002/diff/60001/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/743853002/diff/60001/base/bind_helpers.h#newc...
base/bind_helpers.h:483: // P1 should be the type of the object that will be
received of the method.
On 2014/12/08 19:58:08, Nico wrote:
> nit: "P1" in comment doesn't make sense any more now that P1 is gone

Done.

https://codereview.chromium.org/743853002/diff/60001/base/bind_helpers.h#newc...
base/bind_helpers.h:491: struct IsWeakMethod<true, ConstRefWrapper<WeakPtr<T> >,
Args...>
On 2014/12/08 19:58:08, Nico wrote:
> nit: remove space between > >
> 
> (i think `git cl format` will do this for you)

Done.

https://codereview.chromium.org/743853002/diff/60001/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/743853002/diff/60001/base/bind_internal.h#new...
base/bind_internal.h:62: //                    which grows at O(n^2) with the
arity.
On 2014/12/08 19:58:08, Nico wrote:
> I guess this is still true, except that they're now created by the compiler on
> demand. So I guess the comment still makes sense?

Removed the comment. Assuming our compilers are wise enough to instatiate as few
number of class as possible.

https://codereview.chromium.org/743853002/diff/60001/base/bind_internal.h#new...
base/bind_internal.h:324: template <typename BoundIndexes,
On 2014/12/08 19:58:08, Nico wrote:
> nit: "index" confusingly has two valid plurals, "indices" and "indexes", and
it
> depends on the context which one is correct. I think here it refers to an
index
> in the sense of a subscript, and I believe the right plural in this case is
> "indices"

Done.

https://codereview.chromium.org/743853002/diff/60001/base/bind_internal.h#new...
base/bind_internal.h:329: template <size_t... bound_indexes,
On 2014/12/08 19:58:08, Nico wrote:
> likewise

Done.

[Nico, 2014-12-22 23:07:54]

Sorry, I hadn't realized that you already rebased this on the new Tuple code.
Looking now.

[tzik, 2015-01-09 03:41:25]

On 2014/12/22 23:07:54, Nico wrote:
> Sorry, I hadn't realized that you already rebased this on the new Tuple code.
> Looking now.

ping.

[brucedawson, 2015-01-21 01:14:56]

Can this be updated to the latest code? The patch doesn't seem to apply cleanly
to recent repos, so I can't test this.

[tzik, 2015-01-21 04:21:24]

On 2015/01/21 01:14:56, brucedawson wrote:
> Can this be updated to the latest code? The patch doesn't seem to apply
cleanly
> to recent repos, so I can't test this.

Rebased.

[tzik, 2015-01-21 05:02:52]

On 2015/01/21 04:21:24, tzik wrote:
> On 2015/01/21 01:14:56, brucedawson wrote:
> > Can this be updated to the latest code? The patch doesn't seem to apply
> cleanly
> > to recent repos, so I can't test this.
> 
> Rebased.

+cc:brucedawson

[brucedawson, 2015-01-21 18:21:51]

Thanks. And it works with my change https://codereview.chromium.org/803183004.

[Nico, 2015-02-05 05:53:33]

lgtm

I looked at this (and awong's slides that explain the old implementation) a few
times over the last few weeks, and I think I mostly get it now. I wouldn't say I
_really_ understand what's going on and I very likely couldn't write it, but it
seems correct. (A few of the comments are a few weeks old; ignore the ones that
no longer apply to the current version.)

I wonder if this has any effect on compile time – the compiler needs to parse
less code now, but it needs to do fairly inefficient tuple concats now. I
suppose n is always small in practice and it probably doesn't make a huge
difference either way? Did you have a chance to measure this? (If not, I can
give it a shot tomorrow myself – you can land and I can revert locally)

Thanks much for doing this project, and for your patience!

https://codereview.chromium.org/743853002/diff/180001/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/743853002/diff/180001/base/bind_helpers.h#new...
base/bind_helpers.h:483: // The first argument should be the type of the object
that will be received of
nit: s/received of/received by/

https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h
File base/bind_internal.h (left):

https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h#ol...
base/bind_internal.h:426: // InvokeHelper<>::MakeItSo() call below.
Do you want to keep this comment?

https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h#ne...
base/bind_internal.h:387: InvokeHelper<IsWeakCall::value, R, Runnable,
InvokeHelperArgs>;
All the typedefs above are only used as arguments to Invoker, right? Could they
be private?

https://codereview.chromium.org/743853002/diff/220001/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/743853002/diff/220001/base/bind_helpers.h#new...
base/bind_helpers.h:1: // Copyright (c) 2011 The Chromium Authors. All rights
reserved.
I think the "Introduce Tuple" from the CL description is no longer true.

https://codereview.chromium.org/743853002/diff/220001/base/bind_helpers.h#new...
base/bind_helpers.h:464: scoped_refptr<T> ref_;
nit: Moving this from explicit AddRef / Release to scoped_refptr seems like a
mostly independent change, I would've done that in a different change to keep
this one simpler (but since you've already done it leave it as is)

https://codereview.chromium.org/743853002/diff/220001/base/bind_helpers.h#new...
base/bind_helpers.h:468: struct MaybeScopedRefPtr<true, scoped_refptr<T>,
Rest...> {
Maybe keep the "No need to have a scoped_refptr in the struct since we are
storing a scoped_refptr<> inside the storage object already" comment

[tzik, 2015-02-05 08:38:06]

On 2015/02/05 05:53:33, Nico wrote:
> lgtm
> 
> I looked at this (and awong's slides that explain the old implementation) a
few
> times over the last few weeks, and I think I mostly get it now. I wouldn't say
I
> _really_ understand what's going on and I very likely couldn't write it, but
it
> seems correct. (A few of the comments are a few weeks old; ignore the ones
that
> no longer apply to the current version.)
> 
> I wonder if this has any effect on compile time – the compiler needs to parse
> less code now, but it needs to do fairly inefficient tuple concats now. I
> suppose n is always small in practice and it probably doesn't make a huge
> difference either way? Did you have a chance to measure this? (If not, I can
> give it a shot tomorrow myself – you can land and I can revert locally)
> 
> Thanks much for doing this project, and for your patience!
> 
> https://codereview.chromium.org/743853002/diff/180001/base/bind_helpers.h
> File base/bind_helpers.h (right):
> 
>
https://codereview.chromium.org/743853002/diff/180001/base/bind_helpers.h#new...
> base/bind_helpers.h:483: // The first argument should be the type of the
object
> that will be received of
> nit: s/received of/received by/
> 
> https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h
> File base/bind_internal.h (left):
> 
>
https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h#ol...
> base/bind_internal.h:426: // InvokeHelper<>::MakeItSo() call below.
> Do you want to keep this comment?
> 
> https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h
> File base/bind_internal.h (right):
> 
>
https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h#ne...
> base/bind_internal.h:387: InvokeHelper<IsWeakCall::value, R, Runnable,
> InvokeHelperArgs>;
> All the typedefs above are only used as arguments to Invoker, right? Could
they
> be private?
> 
> https://codereview.chromium.org/743853002/diff/220001/base/bind_helpers.h
> File base/bind_helpers.h (right):
> 
>
https://codereview.chromium.org/743853002/diff/220001/base/bind_helpers.h#new...
> base/bind_helpers.h:1: // Copyright (c) 2011 The Chromium Authors. All rights
> reserved.
> I think the "Introduce Tuple" from the CL description is no longer true.
> 
>
https://codereview.chromium.org/743853002/diff/220001/base/bind_helpers.h#new...
> base/bind_helpers.h:464: scoped_refptr<T> ref_;
> nit: Moving this from explicit AddRef / Release to scoped_refptr seems like a
> mostly independent change, I would've done that in a different change to keep
> this one simpler (but since you've already done it leave it as is)
> 
>
https://codereview.chromium.org/743853002/diff/220001/base/bind_helpers.h#new...
> base/bind_helpers.h:468: struct MaybeScopedRefPtr<true, scoped_refptr<T>,
> Rest...> {
> Maybe keep the "No need to have a scoped_refptr in the struct since we are
> storing a scoped_refptr<> inside the storage object already" comment

There seems no measurable impact to the compile time, at least on linux. :-c

I measured the compile time as follows:
$ ninja -C out/Debug
$ touch base/bind_internal.h
$ time ninja -C out/Debug

All compilations run locally without goma nor ccache.
Then, before this CL is applied:
10107 targets are built, and takes 1139.76s.
# 31143.25s user 1679.63s system 2879% cpu 18:59.76 total
# trace-20150205-1724.json
# https://drive.google.com/a/google.com/file/d/0BwpGjpkRI2N-RG9mM053RFdlRkk

after this CL:
10107 targets are built, and takes 1143.55s.
# 31031.01s user 1675.11s system 2860% cpu 19:03.55 total
# trace-20150205-1655.json
# https://drive.google.com/a/google.com/file/d/0BwpGjpkRI2N-SThtcTVyeGtLV3c

[tzik, 2015-02-05 08:51:52]

New patchsets have been uploaded after l-g-t-m from thakis@chromium.org

[tzik, 2015-02-05 08:52:50]

https://codereview.chromium.org/743853002/diff/180001/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/743853002/diff/180001/base/bind_helpers.h#new...
base/bind_helpers.h:483: // The first argument should be the type of the object
that will be received of
On 2015/02/05 05:53:33, Nico wrote:
> nit: s/received of/received by/

Done.

https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h
File base/bind_internal.h (left):

https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h#ol...
base/bind_internal.h:426: // InvokeHelper<>::MakeItSo() call below.
On 2015/02/05 05:53:33, Nico wrote:
> Do you want to keep this comment?

Done.

https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/743853002/diff/180001/base/bind_internal.h#ne...
base/bind_internal.h:387: InvokeHelper<IsWeakCall::value, R, Runnable,
InvokeHelperArgs>;
On 2015/02/05 05:53:33, Nico wrote:
> All the typedefs above are only used as arguments to Invoker, right? Could
they
> be private?

Done. Yes, they can be private.

https://codereview.chromium.org/743853002/diff/220001/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/743853002/diff/220001/base/bind_helpers.h#new...
base/bind_helpers.h:1: // Copyright (c) 2011 The Chromium Authors. All rights
reserved.
On 2015/02/05 05:53:33, Nico wrote:
> I think the "Introduce Tuple" from the CL description is no longer true.

Done.

https://codereview.chromium.org/743853002/diff/220001/base/bind_helpers.h#new...
base/bind_helpers.h:468: struct MaybeScopedRefPtr<true, scoped_refptr<T>,
Rest...> {
On 2015/02/05 05:53:33, Nico wrote:
> Maybe keep the "No need to have a scoped_refptr in the struct since we are
> storing a scoped_refptr<> inside the storage object already" comment

Done.

[tzik, 2015-02-05 10:22:35]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2015-02-05 10:23:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/743853002/240001

[commit-bot: I haz the power, 2015-02-05 11:40:59]

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

[commit-bot: I haz the power, 2015-02-05 11:41:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nico, 2015-02-05 18:19:23]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2015-02-05 18:20:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/743853002/240001

[commit-bot: I haz the power, 2015-02-05 19:11:59]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-02-05 19:13:20]

Patchset 13 (id:??) landed as
https://crrev.com/8ce65709225bac5922e6b2b80a912cf9796949b1
Cr-Commit-Position: refs/heads/master@{#314847}