tuple: update to make use of C++11

tuple: update to make use of C++11

This CL adds a generic variadic Tuple class that generalizes
Tuple1, Tuple2, etc.  It also adds the C++11-style get<N>(tuple)
method for accessing tuple members.  As a demonstration, the
DispatchToFunction() and DispatchToMethod() functions have been
updated to make use of these and became substantially shorter.

However, to remain compatible with existing code that accesses
Tuple fields by name, the Tuple class is actually implemented via
multiple specializations that match the TupleN classes that used
to exist.  Once all access to foo.a, bar->b, ... have been updated
to get<0>(foo), get<1>(*bar), ... then we can simplify Tuple
further and eventually replace it with std::tuple.

Committed: https://crrev.com/6e7f615f49056439312aad3fcdd2284e2bd69647
Cr-Commit-Position: refs/heads/master@{#307629}

[mdempsky, 2014-11-07 10:16:19]

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

[mdempsky, 2014-11-07 10:16:20]

dcheng: PTAL and let me know what you think of this CL.  If you like the
direction, I'll work on followup CLs to convert existing tuple.x uses to
get<N>(tuple).

[dcheng, 2014-11-07 18:59:35]

I'm generally supportive of CLs that bring us closer to C++11 standard classes.
Does this compile on MSVC? =)

[mdempsky, 2014-11-07 21:39:55]

On 2014/11/07 18:59:35, dcheng wrote:
> I'm generally supportive of CLs that bring us closer to C++11 standard
classes.
> Does this compile on MSVC? =)

I think so: "git cl try" is fully green for the latest patch set, and I think
the Windows bots use MSVC.

[Nico, 2014-11-07 23:26:06]

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

[Nico, 2014-11-07 23:26:07]

The variadic template stuff is cool.

I don't think making tuple similar to standard library features is a good use of
time at this point; from what i can tell Tuple<> isn't used much (so it's easy
to change then, which isn't really true for scoped_ptr)
https://code.google.com/p/chromium/codesearch#search/&q=%5CbTuple%5C%3C%20-fi...
and library support isn't happening super soon.

https://codereview.chromium.org/693693005/diff/40001/base/tuple.h
File base/tuple.h (right):

https://codereview.chromium.org/693693005/diff/40001/base/tuple.h#newcode36
base/tuple.h:36: // Minimal clone of the similarly-named C++14 functionality.
whyyyy

[mdempsky, 2014-11-08 13:38:21]

On 2014/11/07 23:26:07, Nico wrote:
> I don't think making tuple similar to standard library features is a good use
of
> time at this point; from what i can tell Tuple<> isn't used much (so it's easy
> to change then, which isn't really true for scoped_ptr)

My motivation for doing this is I've added / am adding more uses of TupleN, and
there's functionality that I'd like to add (e.g., an operator< as provided by
std::tuple so it can be used as a std::map key).  Implementing those N times
when I could just fix Tuple and implement them once seems silly though.

> https://codereview.chromium.org/693693005/diff/40001/base/tuple.h#newcode36
> base/tuple.h:36: // Minimal clone of the similarly-named C++14 functionality.
> whyyyy

I'm not aware of an alternative way to implement the DispatchToX functions
without integer_sequence, and since we can't yet even use C++11 libraries
(otherwise I would just use std::tuple), I expect it'll be even longer before we
can use C++14.  Hence the need to reimplement it.

I don't think base/tuple.h is the appropriate place for those definitions, but
it was the easiest interim solution before finding out if the overall approach
was acceptable.

[Nico, 2014-11-08 23:14:39]

On Sat, Nov 8, 2014 at 5:38 AM, <mdempsky@chromium.org> wrote:

> On 2014/11/07 23:26:07, Nico wrote:
>
>> I don't think making tuple similar to standard library features is a good
>> use
>>
> of
>
>> time at this point; from what i can tell Tuple<> isn't used much (so it's
>> easy
>> to change then, which isn't really true for scoped_ptr)
>>
>
> My motivation for doing this is I've added / am adding more uses of TupleN


Out of curiosity: What for?


> , and
> there's functionality that I'd like to add (e.g., an operator< as provided
> by
> std::tuple so it can be used as a std::map key).  Implementing those N
> times
> when I could just fix Tuple and implement them once seems silly though.
>
>  https://codereview.chromium.org/693693005/diff/40001/base/
>> tuple.h#newcode36
>> base/tuple.h:36: // Minimal clone of the similarly-named C++14
>> functionality.
>> whyyyy
>>
>
> I'm not aware of an alternative way to implement the DispatchToX functions
> without integer_sequence, and since we can't yet even use C++11 libraries
> (otherwise I would just use std::tuple), I expect it'll be even longer
> before we
> can use C++14.  Hence the need to reimplement it.
>
> I don't think base/tuple.h is the appropriate place for those definitions,
> but
> it was the easiest interim solution before finding out if the overall
> approach
> was acceptable.
>
> https://codereview.chromium.org/693693005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mdempsky, 2014-11-09 01:03:32]

On 2014/11/08 23:14:39, Nico wrote:
> Out of curiosity: What for?

Primarily to write multidimensional maps.  It's much easier to write:

    using MapKey = Tuple4<uint16_t, uint32_t, size_t, size_t>;
    struct MapKeyLess {
      bool operator()(const MapKey& lhs, const MapKey& rhs) const { ... }
    };
    std::map<MapKey, size_t, MapKeyLess> map_;

than to have to implement "struct MapKey".  It would be even nicer to leave out
MapKeyLess.

[Nico, 2014-11-09 23:57:18]

On Sat, Nov 8, 2014 at 5:03 PM, <mdempsky@chromium.org> wrote:

> On 2014/11/08 23:14:39, Nico wrote:
>
>> Out of curiosity: What for?
>>
>
> Primarily to write multidimensional maps.  It's much easier to write:
>
>     using MapKey = Tuple4<uint16_t, uint32_t, size_t, size_t>;
>     struct MapKeyLess {
>       bool operator()(const MapKey& lhs, const MapKey& rhs) const { ... }
>     };
>     std::map<MapKey, size_t, MapKeyLess> map_;
>
> than to have to implement "struct MapKey".
>

Is the tuple really not something that has some meaning in your problem
domain? (In my experience, heavy use of tuples often leads to
hard-to-understand code.)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mdempsky, 2014-11-10 01:02:47]

On 2014/11/09 23:57:18, Nico wrote:
> Is the tuple really not something that has some meaning in your problem
> domain?

Correct.  In both cases, I have only two bits of code that access the tuple: one
where I create the tuple and immediately pass it as an argument to
std::map::insert, and a second where I implement my FooLess functor (which is
entirely generic).

For a representative example, please see CacheKey in this CL:

 
https://codereview.chromium.org/699633003/diff/100001/sandbox/linux/seccomp-b...
 
https://codereview.chromium.org/699633003/diff/100001/sandbox/linux/seccomp-b...

Changing CacheKey from a Tuple typedef to a struct would add about 10 lines of
code to maintain for no value; and once I fix Tuple to provide operator<, then
I'll save another 10 lines of code each time I write code like this.

[Nico, 2014-11-10 02:33:00]

On 2014/11/10 01:02:47, mdempsky (Tokyo until Nov 14) wrote:
> On 2014/11/09 23:57:18, Nico wrote:
> > Is the tuple really not something that has some meaning in your problem
> > domain?
> 
> Correct.  In both cases, I have only two bits of code that access the tuple:
one
> where I create the tuple and immediately pass it as an argument to
> std::map::insert, and a second where I implement my FooLess functor (which is
> entirely generic).
> 
> For a representative example, please see CacheKey in this CL:
> 
>  
>
https://codereview.chromium.org/699633003/diff/100001/sandbox/linux/seccomp-b...
>  
>
https://codereview.chromium.org/699633003/diff/100001/sandbox/linux/seccomp-b...
> 
> Changing CacheKey from a Tuple typedef to a struct would add about 10 lines of
> code to maintain for no value; and once I fix Tuple to provide operator<, then
> I'll save another 10 lines of code each time I write code like this.

But in return, this CL adds 130 lines of compat aliases :-|

[mdempsky, 2014-11-10 02:54:42]

On 2014/11/10 02:33:00, Nico wrote:
> But in return, this CL adds 130 lines of compat aliases :-|

Sigh, forget it.

[tzik, 2014-12-09 12:29:45]

My in-flight CL needs index access to a tuple [1].
It had its own tuple [2], and I wrote alternative change [3] to base::Tuple to
add index support.
Though, this seems to reduce more redundant code than mine and looks closer to
std::tuple.
Is it possible to resurrect this CL?

[1] https://codereview.chromium.org/743853002/
[2] https://codereview.chromium.org/743853002/diff/60001/base/callback_tuple.h
[3] https://codereview.chromium.org/785153002/

[mdempsky, 2014-12-09 22:51:03]

On 2014/12/09 12:29:45, tzik wrote:
> Is it possible to resurrect this CL?

Ask Nico.

[Nico, 2014-12-09 23:03:50]

I think Bind() needing this is a good motivation for this change, so lgtm.

(From what I understand, Tuple needs to get rid of the 8 explicit instantiations
for up to 8 elements for that too, but that can be done in a different cl.)

[mdempsky, 2014-12-09 23:16:00]

On 2014/12/09 23:03:50, Nico wrote:
> I think Bind() needing this is a good motivation for this change, so lgtm.

What do you want me to do about IndexSequence?  Move it to base/index_sequence.h
or something?

[Nico, 2014-12-09 23:27:14]

Do you want to do something with it? I'd probably leave it here for now (?)

[mdempsky, 2014-12-09 23:36:14]

On 2014/12/09 23:27:14, Nico wrote:
> Do you want to do something with it? I'd probably leave it here for now (?)

I don't feel strongly.  I'm fine with leaving it as-is for now, and it can be
revisited later if necessary.  (Just reviewing our dialog to check for any other
outstanding comments.)

Was your LGTM earlier to commit or just to reopen the CL?  Do you want to
re-review it before I mark for CQ?  Should anyone else look at it?

[Nico, 2014-12-10 00:06:14]

On 2014/12/09 23:36:14, mdempsky wrote:
> On 2014/12/09 23:27:14, Nico wrote:
> > Do you want to do something with it? I'd probably leave it here for now (?)
> 
> I don't feel strongly.  I'm fine with leaving it as-is for now, and it can be
> revisited later if necessary.  (Just reviewing our dialog to check for any
other
> outstanding comments.)
> 
> Was your LGTM earlier to commit or just to reopen the CL?  Do you want to
> re-review it before I mark for CQ?  Should anyone else look at it?

lgtm to commit, I looked over the code. I think you can cq (if you're happy with
it yourself). Maybe tzik has comments (he very likely understands this way
better than I do), but if so I think it's ok to address them in a follow-up.

[mdempsky, 2014-12-10 01:07:24]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-12-10 01:07:47]

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

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

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-10 03:11:54]

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

[brucedawson, 2014-12-15 23:22:34]

https://codereview.chromium.org/803183004/ for discussion of the problems that
this change causes for /analyze :-(