tuple: further generalize/simplify Tuple implementation

tuple: further generalize/simplify Tuple implementation

Instead of specially implementing Tuple for 0 to 8 arguments, use a
more traditional implementation by having Tuple<A, B, ...> inherit
from TupleLeaf<0, A>, TupleLeaf<1, B>, etc.  This allows Tuple to be
used with an arbitrary number of arguments.

To remain backwards compatible with code that accesses "t.a", "t.b",
etc., we specialize TupleLeaf for the first 8 elements to store their
values in appropriately named member variables.  These accessors work
even for Tuples with more than 8 elements, but the 9th element and
beyond are only supported with get<N>().

Committed: https://crrev.com/06c626a8091b04618d0859e259f6a53dba6c617a
Cr-Commit-Position: refs/heads/master@{#307681}

[mdempsky, 2014-12-10 05:13:24]

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

[mdempsky, 2014-12-10 05:13:24]

tzik: PTAL.  I was reviewing my variadic template Tuple CL after sending to CQ,
and thought of a few tricks to clean it up further.  Not sure if this helps want
you had in mind or not.

[tzik, 2014-12-10 06:43:27]

LGTM. Cool! I like this even more.

# Please get this reviewed by an OWNER of base/, as usual.

https://codereview.chromium.org/792763002/diff/1/base/tuple.h
File base/tuple.h (right):

https://codereview.chromium.org/792763002/diff/1/base/tuple.h#newcode50
base/tuple.h:50: : public MakeIndexSequenceImpl<N - 1, N - 1, Ns...> {};
can we remove "public" since it's default for structs?

https://codereview.chromium.org/792763002/diff/1/base/tuple.h#newcode207
base/tuple.h:207: typename NthType<I, Ts...>::Type& get(Tuple<Ts...>& tuple) {
If it works, how about matching the tuple against a TupleLeaf directly like
below?

template <size_t I, typename T>
T& GetTupleItem(TupleLeaf<I, T>& tuple) {
  return item.get();
}

[Nico, 2014-12-10 07:00:31]

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

[Nico, 2014-12-10 07:00:32]

Nice!

https://codereview.chromium.org/792763002/diff/1/base/tuple.h
File base/tuple.h (right):

https://codereview.chromium.org/792763002/diff/1/base/tuple.h#newcode100
base/tuple.h:100: Tuple() : TupleBase<Ts...>() {}
Isn't this the same as the constructor in the line below with a 0-element tuple
for args?

https://codereview.chromium.org/792763002/diff/1/base/tuple.h#newcode108
base/tuple.h:108: TupleBaseImpl() : TupleLeaf<Ns, Ts>()... {}
same question

https://codereview.chromium.org/792763002/diff/1/base/tuple.h#newcode128
base/tuple.h:128: // For legacy compatibility, we name the first 8 tuple
elements "a", "b", ...
Does it make sense to remove all clients of this and then remove the code for
this? If so, file a bug for that and add a TODO with a link to the bug. (You
don't have to do that todo yourself.)

[Nico, 2014-12-10 07:01:29]

actually, to keep things moving lgtm with answers to my questions in the
previous reply

[mdempsky, 2014-12-10 07:15:23]

(Clarifying/answering this one question before working on the other feedback.)

https://codereview.chromium.org/792763002/diff/1/base/tuple.h
File base/tuple.h (right):

https://codereview.chromium.org/792763002/diff/1/base/tuple.h#newcode100
base/tuple.h:100: Tuple() : TupleBase<Ts...>() {}
On 2014/12/10 07:00:31, Nico wrote:
> Isn't this the same as the constructor in the line below with a 0-element
tuple
> for args?

If I understand your question: "for Tuple<>, would the constructors on lines 100
and 101 be the same?"?

Yes, it would become effectively:

  100  Tuple() : TupleBase<>() {}
  101  explicit Tuple() : TupleBase<>() {}

which is why I needed the Tuple<> specialization on lines 124--126.  Otherwise a
call to Tuple<>() is ambiguous because it could be either of these, so the
compiler complains (even though they're semantically equivalent).

(And assuming this answers your question here, I think it answers your question
on line 108 too.)

[mdempsky, 2014-12-10 07:46:03]

Thanks!

https://codereview.chromium.org/792763002/diff/1/base/tuple.h
File base/tuple.h (right):

https://codereview.chromium.org/792763002/diff/1/base/tuple.h#newcode50
base/tuple.h:50: : public MakeIndexSequenceImpl<N - 1, N - 1, Ns...> {};
On 2014/12/10 06:43:27, tzik wrote:
> can we remove "public" since it's default for structs?

Done (here and below).

https://codereview.chromium.org/792763002/diff/1/base/tuple.h#newcode100
base/tuple.h:100: Tuple() : TupleBase<Ts...>() {}
(On the chance that issue was the Tuple<> specialization wasn't obvious because
it was too far away, I've moved it closer to here.)

https://codereview.chromium.org/792763002/diff/1/base/tuple.h#newcode128
base/tuple.h:128: // For legacy compatibility, we name the first 8 tuple
elements "a", "b", ...
On 2014/12/10 07:00:31, Nico wrote:
> Does it make sense to remove all clients of this and then remove the code for
> this? If so, file a bug for that and add a TODO with a link to the bug. (You
> don't have to do that todo yourself.)

Done.

https://codereview.chromium.org/792763002/diff/1/base/tuple.h#newcode207
base/tuple.h:207: typename NthType<I, Ts...>::Type& get(Tuple<Ts...>& tuple) {
On 2014/12/10 06:43:27, tzik wrote:
> If it works, how about matching the tuple against a TupleLeaf directly like
> below?
> 
> template <size_t I, typename T>
> T& GetTupleItem(TupleLeaf<I, T>& tuple) {
>   return item.get();
> }

Nice, seems to work, and means NthType isn't needed anymore either.

Only downside is it's perhaps less obvious to users that they can use it on a
Tuple, so I added some documentation to demonstrate that.

[tzik, 2014-12-10 09:01:06]

still LGTM.

[mdempsky, 2014-12-10 09:45:19]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-12-10 09:46:10]

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

[commit-bot: I haz the power, 2014-12-10 11:22:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-10 11:23:19]

Patchset 3 (id:??) landed as
https://crrev.com/06c626a8091b04618d0859e259f6a53dba6c617a
Cr-Commit-Position: refs/heads/master@{#307681}