Bug 48476 - [C++0x] conversion between std::tuple which have reference member is rejected
Summary: [C++0x] conversion between std::tuple which have reference member is rejected
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.6.0
: P3 normal
Target Milestone: 4.6.1
Assignee: Not yet assigned to anyone
URL:
Keywords: rejects-valid
Depends on:
Blocks:
 
Reported: 2011-04-06 02:32 UTC by Takaya Saito
Modified: 2011-04-15 14:54 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-04-06 16:29:15


Attachments
Proposed patch for libstdc++-v3/include/std/tuple.hpp (338 bytes, application/octet-stream)
2011-04-06 02:32 UTC, Takaya Saito
Details
simple test for operator=( Tuple&& ) (296 bytes, text/plain)
2011-04-06 08:32 UTC, Takaya Saito
Details
simple test for std::tuple_cat (455 bytes, text/plain)
2011-04-06 08:32 UTC, Takaya Saito
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Takaya Saito 2011-04-06 02:32:58 UTC
Created attachment 23893 [details]
Proposed patch for libstdc++-v3/include/std/tuple.hpp

gcc-4.6.0 rejects this code (bug.cc):

---------------------

#include <tuple>

int main()
{
  int i = 0;
  std::tuple<int&, int> t = std::forward_as_tuple( i, 0 );
}

---------------------

# g++ -std=c++0x bug.cc

In file included from bug.cc:1:0:
/usr/local/gcc46/lib/gcc/i686-pc-cygwin/4.6.0/../../../../include/c++/4.6.0/tuple: In constructor 'std::_Head_base<_Idx, _Head, false>::_Head_base(_UHead&&) [with _UHead = int, unsigned int _Idx = 0u, _Head = int&]':
/usr/local/gcc46/lib/gcc/i686-pc-cygwin/4.6.0/../../../../include/c++/4.6.0/tuple:183:35:   instantiated from 'std::_Tuple_impl<_Idx, _Head, _Tail ...>::_Tuple_impl(std::_Tuple_impl<_Idx, _UElements ...>&&) [with _UElements = {int&, int&&}, unsigned int _Idx = 0u, _Head = int&, _Tail = {int}]'
/usr/local/gcc46/lib/gcc/i686-pc-cygwin/4.6.0/../../../../include/c++/4.6.0/tuple:343:60:   instantiated from 'std::tuple<_T1, _T2>::tuple(std::tuple<_U1, _U2>&&) [with _U1 = int&, _U2 = int&&, _T1 = int&, _T2 = int]'
bug.cc:6:57:   instantiated from here
/usr/local/gcc46/lib/gcc/i686-pc-cygwin/4.6.0/../../../../include/c++/4.6.0/tuple:101:42: error: invalid initialization of non-const reference of type 'int&' from an rvalue of type 'int'

---------------------


I found a conversion ctor of '_Tuple_impl<_Idx, _Head, _Tail...>',

---------------------

// libstdc++-v3/include/std/tuple.hpp, line 180

template<typename... _UElements>
  _Tuple_impl(_Tuple_impl<_Idx, _UElements...>&& __in)
  : _Inherited(std::move(__in._M_tail())),
    _Base(std::move(__in._M_head())) { }

---------------------

is wrong, when the first type of '_UElements...' is lvalue reference.
It should be implemented with std::forward, not std::move, like this:

---------------------

template<typename _UHead, typename... _UTails>
  _Tuple_impl(_Tuple_impl<_Idx, _UHead, _UTails...>&& __in)
  : _Inherited(std::move(__in._M_tail())),
    _Base(std::forward<_UHead>(__in._M_head())) { }

---------------------

and it would be work better.


And I found these kinds of bugs also appear in implementations of assignment-ops
and std::tuple_cat, so I attach a brief patch to fix them ( it might have
some problems, because I have not tested it enough ).
I'm glad if it could be some help.
Comment 1 Paolo Carlini 2011-04-06 07:38:39 UTC
Hi. Not having looked into it in detail yet, your patch looks indeed reasonable. Do you have more testcases, for at lease some of the other issues you noticed?

Indeed, I really meant to go again through std::tuple, I was nervous about some of those move and forward... Can you double check tuple_cat and tuple itself about that? I'm seeing some quite suspect moves, compared to the current draft. Thanks.
Comment 2 Takaya Saito 2011-04-06 08:32:09 UTC
Created attachment 23896 [details]
simple test for operator=( Tuple&& )
Comment 3 Takaya Saito 2011-04-06 08:32:57 UTC
Created attachment 23897 [details]
simple test for std::tuple_cat
Comment 4 Takaya Saito 2011-04-06 09:05:19 UTC
Comment on attachment 23896 [details]
simple test for operator=( Tuple&& )

in gcc-4.6.0, this code fails assertion 'q == p' and 'r == q',
because references to p and q are not forwarded but moved
( see N3242 20.4.2.2 ).
Comment 5 Takaya Saito 2011-04-06 09:07:03 UTC
Comment on attachment 23897 [details]
simple test for std::tuple_cat

gcc-4.6.0 rejects this code.
Comment 6 Paolo Carlini 2011-04-06 14:07:19 UTC
Ok, thanks. Still, I believe we have other std::move which should be turned into forward, in std::tuple. Those in std::tuple_cat itself also seem suspect, I see you are touching only the helper.
Comment 7 Takaya Saito 2011-04-06 15:32:00 UTC
(In reply to comment #6)
> Ok, thanks. Still, I believe we have other std::move which should be turned
> into forward, in std::tuple. Those in std::tuple_cat itself also seem suspect,
> I see you are touching only the helper.

Well, I think I had replaced all std::move that should be turned into forward ;
remaining std::move are moving _Inherited ( _Tuple_impl<_Idx + 1, _Tail...> ), 
tuple<Elements...> or rvalue reference to these classes, which are not lvalue reference type, so they are not to be replaced ( of course, it is not wrong
to replace them with std::forward, since std::move(x) is equivalent to
std::forward<T>(x) if T is remove_reference<decltype((x))>::type ).
Comment 8 Paolo Carlini 2011-04-06 16:29:15 UTC
Fair enough. I think the safe thing to do here is proceeding incrementally: today I will be testing your patch (+ testcases) and I think it's small enough to bd suited for 4_6-branch too. Then we can certainly imagine other improvements. If you notice something else which could be improved over the next weeks, please let us know, I mean to pay more attention to std::tuple than I used to lately, sadly.
Comment 9 Paolo Carlini 2011-04-12 09:10:38 UTC
On it.
Comment 10 paolo@gcc.gnu.org 2011-04-12 10:31:37 UTC
Author: paolo
Date: Tue Apr 12 10:31:33 2011
New Revision: 172309

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=172309
Log:
2011-04-12  Takaya Saito  <gintensubaru@gmail.com>

	PR libstdc++/48476
	* include/std/tuple (_Tuple_impl<>::_Tuple_impl(_Tuple_impl<>&&),
	_Tuple_impl<>::operator=(_Tuple_impl&&), _Tuple_impl<>::operator=
	(_Tuple_impl<>&&), tuple_cat): Use std::forward where appropriate.
	* testsuite/20_util/tuple/cons/48476.cc: New.
	* testsuite/20_util/tuple/48476.cc: Likewise.
	* testsuite/20_util/tuple/creation_functions/48476.cc: Likewise.

Added:
    trunk/libstdc++-v3/testsuite/20_util/tuple/48476.cc
    trunk/libstdc++-v3/testsuite/20_util/tuple/cons/48476.cc
    trunk/libstdc++-v3/testsuite/20_util/tuple/creation_functions/48476.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/std/tuple
Comment 11 paolo@gcc.gnu.org 2011-04-15 14:53:02 UTC
Author: paolo
Date: Fri Apr 15 14:52:57 2011
New Revision: 172498

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=172498
Log:
2011-04-15  Takaya Saito  <gintensubaru@gmail.com>

	PR libstdc++/48476
	* include/std/tuple (_Tuple_impl<>::_Tuple_impl(_Tuple_impl<>&&),
	_Tuple_impl<>::operator=(_Tuple_impl&&), _Tuple_impl<>::operator=
	(_Tuple_impl<>&&), tuple_cat): Use std::forward where appropriate.
	* testsuite/20_util/tuple/cons/48476.cc: New.
	* testsuite/20_util/tuple/48476.cc: Likewise.
	* testsuite/20_util/tuple/creation_functions/48476.cc: Likewise.

Added:
    branches/gcc-4_6-branch/libstdc++-v3/testsuite/20_util/tuple/48476.cc
    branches/gcc-4_6-branch/libstdc++-v3/testsuite/20_util/tuple/cons/48476.cc
    branches/gcc-4_6-branch/libstdc++-v3/testsuite/20_util/tuple/creation_functions/48476.cc
Modified:
    branches/gcc-4_6-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_6-branch/libstdc++-v3/include/std/tuple
Comment 12 Paolo Carlini 2011-04-15 14:54:10 UTC
Done.