Add base::get to support rvalue-reference tuple

Add base::get to support rvalue-reference tuple

std::get in libstdc++4.6 doesn't have a overload for rvalue-reference
tuple. That implies std::get<0>(std::make_tuple(1)) is `const int&`,
instead of `int&&`.
This CL add base::get with correct type.

Committed: https://crrev.com/ec749b1eeaa49a45973e651e870ed16531c8b0d9
Cr-Commit-Position: refs/heads/master@{#401758}

[tzik, 2016-06-07 16:08:47]

Description was changed from

==========
Add base::get to get tuple elements

std::get in libstdc++4.6 doesn't have a overload for rvalue-reference
tuple. That implies std::get<0>(std::make_tuple(1)) is `const int&`,
instead of `int&&`.
This CL add base::get with correct type.
==========

to

==========
Add base::get to support rvalue-reference tuple

std::get in libstdc++4.6 doesn't have a overload for rvalue-reference
tuple. That implies std::get<0>(std::make_tuple(1)) is `const int&`,
instead of `int&&`.
This CL add base::get with correct type.
==========

[tzik, 2016-06-07 16:08:47]

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

[tzik, 2016-06-07 16:09:09]

PTAL

[tzik, 2016-06-10 06:28:21]

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

[tzik, 2016-06-10 06:28:22]

Dana, could you review this?

[danakj, 2016-06-14 18:54:07]

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

https://codereview.chromium.org/2047013002/diff/1/base/tuple.h#newcode113
base/tuple.h:113: return std::forward<ElemType>(std::get<I>(t));
Why forward instead of move?

Trying to compare to
https://github.com/llvm-mirror/libcxx/blob/master/include/tuple#L979, though
you're using std::get() here of course. They static cast to ElemType&&.

It seems to work with forward from the tests, but if move is the same in all
cases, I prefer that.

[danakj, 2016-06-14 18:54:52]

Oh, can you also add a note in styleguide/c++/ under tuple about this at the
same time?

[tzik, 2016-06-14 22:34:52]

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

https://codereview.chromium.org/2047013002/diff/1/base/tuple.h#newcode113
base/tuple.h:113: return std::forward<ElemType>(std::get<I>(t));
On 2016/06/14 18:54:07, danakj wrote:
> Why forward instead of move?
> 
> Trying to compare to
> https://github.com/llvm-mirror/libcxx/blob/master/include/tuple#L979, though
> you're using std::get() here of course. They static cast to ElemType&&.
> 
> It seems to work with forward from the tests, but if move is the same in all
> cases, I prefer that.

This is needed when the ElemType is a lvalue-reference.
In the case below:
  int i = 0;
  base::get(std::tie(i));
std::tie(i) returns std::tuple<int&>&&, and base::get should return `int&`.
If we use std::move in the base::get impl, the return type will be `int&&` and
|i| above will be moved unexpectedly.

[danakj, 2016-06-17 22:55:12]

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

https://codereview.chromium.org/2047013002/diff/1/base/tuple.h#newcode113
base/tuple.h:113: return std::forward<ElemType>(std::get<I>(t));
On 2016/06/14 22:34:52, tzik wrote:
> On 2016/06/14 18:54:07, danakj wrote:
> > Why forward instead of move?
> > 
> > Trying to compare to
> > https://github.com/llvm-mirror/libcxx/blob/master/include/tuple#L979, though
> > you're using std::get() here of course. They static cast to ElemType&&.
> > 
> > It seems to work with forward from the tests, but if move is the same in all
> > cases, I prefer that.
> 
> This is needed when the ElemType is a lvalue-reference.
> In the case below:
>   int i = 0;
>   base::get(std::tie(i));
> std::tie(i) returns std::tuple<int&>&&, and base::get should return `int&`.
> If we use std::move in the base::get impl, the return type will be `int&&` and
> |i| above will be moved unexpectedly.

Hm. But isn't the return type already hardcoded to type&& above? And in the
example you gave, it would use the get() method below this to simply forward to
std::get()?

[tzik, 2016-06-21 08:46:33]

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

https://codereview.chromium.org/2047013002/diff/1/base/tuple.h#newcode113
base/tuple.h:113: return std::forward<ElemType>(std::get<I>(t));
On 2016/06/17 22:55:12, danakj wrote:
> On 2016/06/14 22:34:52, tzik wrote:
> > On 2016/06/14 18:54:07, danakj wrote:
> > > Why forward instead of move?
> > > 
> > > Trying to compare to
> > > https://github.com/llvm-mirror/libcxx/blob/master/include/tuple#L979,
though
> > > you're using std::get() here of course. They static cast to ElemType&&.
> > > 
> > > It seems to work with forward from the tests, but if move is the same in
all
> > > cases, I prefer that.
> > 
> > This is needed when the ElemType is a lvalue-reference.
> > In the case below:
> >   int i = 0;
> >   base::get(std::tie(i));
> > std::tie(i) returns std::tuple<int&>&&, and base::get should return `int&`.
> > If we use std::move in the base::get impl, the return type will be `int&&`
and
> > |i| above will be moved unexpectedly.
> 
> Hm. But isn't the return type already hardcoded to type&& above? And in the
> example you gave, it would use the get() method below this to simply forward
to
> std::get()?

I added a test case to reveal std::move doesn't work here: the previous ones
test only decltype() of base::get(), and didn't catch the compilation error in
the impl of base::get().

Though `type&&` looks like a rvalue-reference, it can be lvalue-reference when
|type| is already a lvalue-reference.
  using T = int&;
  static_assert(std::is_same<int&, T&&>::value, "");

In my previous example, since std::tie() returns the tuple by value, the first
overload of base::get() handles it rather than the second.

[danakj, 2016-06-22 22:37:50]

OK I get it, sorry I was stumbling over the many types in the function
signature, the return type is the ::type of the tuple *element*, where as the
parameter is the type of the tuple itself. Thanks for the explanation with
std::tie, as well.

https://codereview.chromium.org/2047013002/diff/40001/styleguide/c++/c++11.html
File styleguide/c++/c++11.html (left):

https://codereview.chromium.org/2047013002/diff/40001/styleguide/c++/c++11.ht...
styleguide/c++/c++11.html:457: <td>Prefer <code>std::tuple</code> over <a
href="https://crbug.com/554987"><code>base::Tuple</code></a>.</td>
I think you want to keep this line, we do prefer using std::tuple. Just add a
note about get() being broken to this.

[tzik, 2016-06-23 04:37:30]

https://codereview.chromium.org/2047013002/diff/40001/styleguide/c++/c++11.html
File styleguide/c++/c++11.html (left):

https://codereview.chromium.org/2047013002/diff/40001/styleguide/c++/c++11.ht...
styleguide/c++/c++11.html:457: <td>Prefer <code>std::tuple</code> over <a
href="https://crbug.com/554987"><code>base::Tuple</code></a>.</td>
On 2016/06/22 22:37:49, danakj wrote:
> I think you want to keep this line, we do prefer using std::tuple. Just add a
> note about get() being broken to this.

We already replaced all usage of base::Tuple with std::tuple, and removed it.
Do we still want to mention base::Tuple?

[danakj, 2016-06-23 20:41:05]

LGTM

https://codereview.chromium.org/2047013002/diff/40001/styleguide/c++/c++11.html
File styleguide/c++/c++11.html (left):

https://codereview.chromium.org/2047013002/diff/40001/styleguide/c++/c++11.ht...
styleguide/c++/c++11.html:457: <td>Prefer <code>std::tuple</code> over <a
href="https://crbug.com/554987"><code>base::Tuple</code></a>.</td>
On 2016/06/23 04:37:29, tzik wrote:
> On 2016/06/22 22:37:49, danakj wrote:
> > I think you want to keep this line, we do prefer using std::tuple. Just add
a
> > note about get() being broken to this.
> 
> We already replaced all usage of base::Tuple with std::tuple, and removed it.
> Do we still want to mention base::Tuple?

Ooh, okay. Woops.

[danakj, 2016-06-23 20:41:08]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-06-23 20:41:36]

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

[commit-bot: I haz the power, 2016-06-24 00:33:58]

Description was changed from

==========
Add base::get to support rvalue-reference tuple

std::get in libstdc++4.6 doesn't have a overload for rvalue-reference
tuple. That implies std::get<0>(std::make_tuple(1)) is `const int&`,
instead of `int&&`.
This CL add base::get with correct type.
==========

to

==========
Add base::get to support rvalue-reference tuple

std::get in libstdc++4.6 doesn't have a overload for rvalue-reference
tuple. That implies std::get<0>(std::make_tuple(1)) is `const int&`,
instead of `int&&`.
This CL add base::get with correct type.
==========

[commit-bot: I haz the power, 2016-06-24 00:33:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-24 00:35:48]

Description was changed from

==========
Add base::get to support rvalue-reference tuple

std::get in libstdc++4.6 doesn't have a overload for rvalue-reference
tuple. That implies std::get<0>(std::make_tuple(1)) is `const int&`,
instead of `int&&`.
This CL add base::get with correct type.
==========

to

==========
Add base::get to support rvalue-reference tuple

std::get in libstdc++4.6 doesn't have a overload for rvalue-reference
tuple. That implies std::get<0>(std::make_tuple(1)) is `const int&`,
instead of `int&&`.
This CL add base::get with correct type.

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

[commit-bot: I haz the power, 2016-06-24 00:35:49]

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