diff mbox

tuple move constructor

Message ID alpine.DEB.2.02.1605232038220.30609@laptop-mg.saclay.inria.fr
State New
Headers show

Commit Message

Marc Glisse May 23, 2016, 6:39 p.m. UTC 
Ping

(re-attaching, I just added a one-line comment before the tag class as 
asked by Ville)

On Thu, 21 Apr 2016, Marc Glisse wrote:

> On Thu, 21 Apr 2016, Jonathan Wakely wrote:
>
>> On 20 April 2016 at 21:42, Marc Glisse wrote:
>>> Hello,
>>> 
>>> does anyone remember why the move constructor of _Tuple_impl is not
>>> defaulted? The attached patch does not cause any test to fail (whitespace
>>> kept to avoid line number changes). Maybe something about tuples of
>>> references?
>> 
>> I don't know/remember why. It's possible it was to workaround a
>> front-end bug that required it, or maybe just a mistake and it should
>> always have been defaulted.
>
> Ok, then how about something like this? In order to suppress the move
> constructor in tuple (when there is a non-movable element), we need to
> either declare it with suitable constraints, or keep it defaulted and
> ensure that we don't bypass a missing move constructor anywhere along
> the way (_Tuple_impl, _Head_base). There is a strange mix of 2
> strategies in the patch, I prefer the tag class, but I started using
> enable_if before I realized how many places needed those horrors.
>
> Bootstrap+regtest on powerpc64le-unknown-linux-gnu.
>
>
> 2016-04-22  Marc Glisse  <marc.glisse@inria.fr>
>
> 	* include/std/tuple (__element_arg_t): New class.
> 	(_Head_base(const _Head&), _Tuple_impl(const _Head&, const _Tail&...):
> 	Remove.
> 	(_Head_base(_UHead&&)): Add __element_arg_t argument...
> 	(_Tuple_impl): ... and adjust callers.
> 	(_Tuple_impl(_Tuple_impl&&)): Default.
> 	(_Tuple_impl(const _Tuple_impl<other>&),
> 	_Tuple_impl(_Tuple_impl<other>&&), _Tuple_impl(_UHead&&): Constrain.
> 	* testsuite/20_util/tuple/nomove.cc: New.

Comments

Jonathan Wakely May 25, 2016, 1:54 p.m. UTC | #1 
On 23/05/16 20:39 +0200, Marc Glisse wrote:
>Ping
>
>(re-attaching, I just added a one-line comment before the tag class as 
>asked by Ville)

This is OK for trunk - thanks.
Jonathan Wakely May 26, 2016, 3:37 p.m. UTC | #2 
On 25/05/16 14:54 +0100, Jonathan Wakely wrote:
>On 23/05/16 20:39 +0200, Marc Glisse wrote:
>>Ping
>>
>>(re-attaching, I just added a one-line comment before the tag class 
>>as asked by Ville)
>
>This is OK for trunk - thanks.

On second thoughts - does this change the passing conventions for
std::tuple if it gets a trivial move ctor?
Marc Glisse May 26, 2016, 5:49 p.m. UTC | #3 
On Thu, 26 May 2016, Jonathan Wakely wrote:

> On 25/05/16 14:54 +0100, Jonathan Wakely wrote:
>> On 23/05/16 20:39 +0200, Marc Glisse wrote:
>>> Ping
>>> 
>>> (re-attaching, I just added a one-line comment before the tag class as 
>>> asked by Ville)
>> 
>> This is OK for trunk - thanks.
>
> On second thoughts - does this change the passing conventions for
> std::tuple if it gets a trivial move ctor?

Note that this part of the ABI is ill-defined
http://sourcerytools.com/pipermail/cxx-abi-dev/2016-February/002884.html

but yes, good catch, it does change the passing convention (by value), and 
not just for weirdo types, it even changes for tuple<int>. It is clearly a 
change in the right direction, not passing tuple<int> in a register is 
weird, but yeah, compatibility :-(

I don't even want to think of trying to fix this issue in C++11 while 
artificially preserving the non-triviality of tuple, the headache is not 
worth it. I guess I'll open an entry in bugzilla with the ABI tag and let 
it rot there...

Maybe we could

#if __cpp_concepts >= 201500
the alternative discussed with Ville
#endif

but that won't fix the fact that tuple<int> should be trivially move 
constructible...

We could add __attribute__(non_trivial_for_purpose_of_passing_convention), 
but I think abi_tag has already stretched enough the idea that gcc is 
following the itanium abi.

Bah, forget this patch. Thanks for noticing early, that spares me the 
trouble of reverting later.
Jonathan Wakely May 26, 2016, 5:56 p.m. UTC | #4 
On 26/05/16 19:49 +0200, Marc Glisse wrote:
>On Thu, 26 May 2016, Jonathan Wakely wrote:
>
>>On 25/05/16 14:54 +0100, Jonathan Wakely wrote:
>>>On 23/05/16 20:39 +0200, Marc Glisse wrote:
>>>>Ping
>>>>
>>>>(re-attaching, I just added a one-line comment before the tag 
>>>>class as asked by Ville)
>>>
>>>This is OK for trunk - thanks.
>>
>>On second thoughts - does this change the passing conventions for
>>std::tuple if it gets a trivial move ctor?
>
>Note that this part of the ABI is ill-defined
>http://sourcerytools.com/pipermail/cxx-abi-dev/2016-February/002884.html
>
>but yes, good catch, it does change the passing convention (by value), 
>and not just for weirdo types, it even changes for tuple<int>. It is 
>clearly a change in the right direction, not passing tuple<int> in a 
>register is weird, but yeah, compatibility :-(
>
>I don't even want to think of trying to fix this issue in C++11 while 
>artificially preserving the non-triviality of tuple, the headache is 
>not worth it. I guess I'll open an entry in bugzilla with the ABI tag 
>and let it rot there...

Please do.

>Maybe we could
>
>#if __cpp_concepts >= 201500
>the alternative discussed with Ville
>#endif
>
>but that won't fix the fact that tuple<int> should be trivially move 
>constructible...
>
>We could add 
>__attribute__(non_trivial_for_purpose_of_passing_convention), but I 
>think abi_tag has already stretched enough the idea that gcc is 
>following the itanium abi.
>
>Bah, forget this patch. Thanks for noticing early, that spares me the 
>trouble of reverting later.

I have a dream of resurrecting the gnu-versioned-namespace config (and
bumping from std::__7 / libstdc++.so.7 to std::__8 / libstdc++.so.8)
so that people who don't want backwards compatibility can use that
mode, which would enable various nice optimizations that can't be made
in the default mode because of compatibility.

This would be something we should change when that config is in use.
So hopefully if you open a bugzilla entry it won't rot forever, but
only until I realise my dream.
diff mbox

Patch

Index: libstdc++-v3/include/std/tuple
===================================================================
--- libstdc++-v3/include/std/tuple	(revision 236338)
+++ libstdc++-v3/include/std/tuple	(working copy)
@@ -41,38 +41,38 @@ 
 
 namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   /**
    *  @addtogroup utilities
    *  @{
    */
 
+  // Tag type to distinguish forwarding constructors from copy/move.
+  struct __element_arg_t { };
+
   template<std::size_t _Idx, typename _Head, bool _IsEmptyNotFinal>
     struct _Head_base;
 
   template<std::size_t _Idx, typename _Head>
     struct _Head_base<_Idx, _Head, true>
     : public _Head
     {
       constexpr _Head_base()
       : _Head() { }
 
-      constexpr _Head_base(const _Head& __h)
-      : _Head(__h) { }
-
       constexpr _Head_base(const _Head_base&) = default;
       constexpr _Head_base(_Head_base&&) = default;
 
       template<typename _UHead>
-        constexpr _Head_base(_UHead&& __h)
+        constexpr _Head_base(__element_arg_t, _UHead&& __h)
 	: _Head(std::forward<_UHead>(__h)) { }
 
       _Head_base(allocator_arg_t, __uses_alloc0)
       : _Head() { }
 
       template<typename _Alloc>
 	_Head_base(allocator_arg_t, __uses_alloc1<_Alloc> __a)
 	: _Head(allocator_arg, *__a._M_a) { }
 
       template<typename _Alloc>
@@ -97,28 +97,25 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       static constexpr const _Head&
       _M_head(const _Head_base& __b) noexcept { return __b; }
     };
 
   template<std::size_t _Idx, typename _Head>
     struct _Head_base<_Idx, _Head, false>
     {
       constexpr _Head_base()
       : _M_head_impl() { }
 
-      constexpr _Head_base(const _Head& __h)
-      : _M_head_impl(__h) { }
-
       constexpr _Head_base(const _Head_base&) = default;
       constexpr _Head_base(_Head_base&&) = default;
 
       template<typename _UHead>
-        constexpr _Head_base(_UHead&& __h)
+        constexpr _Head_base(__element_arg_t, _UHead&& __h)
 	: _M_head_impl(std::forward<_UHead>(__h)) { }
 
       _Head_base(allocator_arg_t, __uses_alloc0)
       : _M_head_impl() { }
 
       template<typename _Alloc>
 	_Head_base(allocator_arg_t, __uses_alloc1<_Alloc> __a)
 	: _M_head_impl(allocator_arg, *__a._M_a) { }
 
       template<typename _Alloc>
@@ -194,50 +191,49 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       static constexpr _Inherited&
       _M_tail(_Tuple_impl& __t) noexcept { return __t; }
 
       static constexpr const _Inherited&
       _M_tail(const _Tuple_impl& __t) noexcept { return __t; }
 
       constexpr _Tuple_impl()
       : _Inherited(), _Base() { }
 
-      explicit 
-      constexpr _Tuple_impl(const _Head& __head, const _Tail&... __tail)
-      : _Inherited(__tail...), _Base(__head) { }
-
       template<typename _UHead, typename... _UTail, typename = typename
                enable_if<sizeof...(_Tail) == sizeof...(_UTail)>::type> 
         explicit
         constexpr _Tuple_impl(_UHead&& __head, _UTail&&... __tail)
 	: _Inherited(std::forward<_UTail>(__tail)...),
-	  _Base(std::forward<_UHead>(__head)) { }
+	  _Base(__element_arg_t(), std::forward<_UHead>(__head)) { }
 
       constexpr _Tuple_impl(const _Tuple_impl&) = default;
+      constexpr _Tuple_impl(_Tuple_impl&&) = default;
 
-      constexpr
-      _Tuple_impl(_Tuple_impl&& __in)
-      noexcept(__and_<is_nothrow_move_constructible<_Head>,
-	              is_nothrow_move_constructible<_Inherited>>::value)
-      : _Inherited(std::move(_M_tail(__in))), 
-	_Base(std::forward<_Head>(_M_head(__in))) { }
-
-      template<typename... _UElements>
+      template<typename... _UElements,
+	typename enable_if<
+	  !is_same<_Tuple_impl,
+		   _Tuple_impl<_Idx, _UElements...>>::value,
+	  bool>::type = false>
         constexpr _Tuple_impl(const _Tuple_impl<_Idx, _UElements...>& __in)
 	: _Inherited(_Tuple_impl<_Idx, _UElements...>::_M_tail(__in)),
-	  _Base(_Tuple_impl<_Idx, _UElements...>::_M_head(__in)) { }
+	  _Base(__element_arg_t(),
+		_Tuple_impl<_Idx, _UElements...>::_M_head(__in)) { }
 
-      template<typename _UHead, typename... _UTails>
+      template<typename _UHead, typename... _UTails,
+	       typename enable_if<
+		 !is_same<_Tuple_impl,
+			  _Tuple_impl<_Idx, _UHead, _UTails...>>::value,
+		 bool>::type = false>
         constexpr _Tuple_impl(_Tuple_impl<_Idx, _UHead, _UTails...>&& __in)
 	: _Inherited(std::move
 		     (_Tuple_impl<_Idx, _UHead, _UTails...>::_M_tail(__in))),
-	  _Base(std::forward<_UHead>
+	  _Base(__element_arg_t(), std::forward<_UHead>
 		(_Tuple_impl<_Idx, _UHead, _UTails...>::_M_head(__in))) { }
 
       template<typename _Alloc>
 	_Tuple_impl(allocator_arg_t __tag, const _Alloc& __a)
 	: _Inherited(__tag, __a),
           _Base(__tag, __use_alloc<_Head>(__a)) { }
 
       template<typename _Alloc>
 	_Tuple_impl(allocator_arg_t __tag, const _Alloc& __a,
 		    const _Head& __head, const _Tail&... __tail)
@@ -344,43 +340,42 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       static constexpr _Head&
       _M_head(_Tuple_impl& __t) noexcept { return _Base::_M_head(__t); }
 
       static constexpr const _Head&
       _M_head(const _Tuple_impl& __t) noexcept { return _Base::_M_head(__t); }
 
       constexpr _Tuple_impl()
       : _Base() { }
 
-      explicit
-      constexpr _Tuple_impl(const _Head& __head)
-      : _Base(__head) { }
-
-      template<typename _UHead>
+      template<typename _UHead, typename enable_if<
+		!is_same<_Tuple_impl,
+		  typename remove_cv<
+		    typename remove_reference<_UHead>::type>::type>::value,
+		bool>::type = false>
         explicit
         constexpr _Tuple_impl(_UHead&& __head)
-	: _Base(std::forward<_UHead>(__head)) { }
+	: _Base(__element_arg_t(), std::forward<_UHead>(__head)) { }
 
       constexpr _Tuple_impl(const _Tuple_impl&) = default;
+      constexpr _Tuple_impl(_Tuple_impl&&) = default;
 
-      constexpr
-      _Tuple_impl(_Tuple_impl&& __in)
-      noexcept(is_nothrow_move_constructible<_Head>::value)
-      : _Base(std::forward<_Head>(_M_head(__in))) { }
-
-      template<typename _UHead>
+      template<typename _UHead, typename enable_if<
+		!is_same<_UHead, _Head>::value, bool>::type = false>
         constexpr _Tuple_impl(const _Tuple_impl<_Idx, _UHead>& __in)
-	: _Base(_Tuple_impl<_Idx, _UHead>::_M_head(__in)) { }
+	: _Base(__element_arg_t(), _Tuple_impl<_Idx, _UHead>::_M_head(__in)) { }
 
-      template<typename _UHead>
+      template<typename _UHead, typename enable_if<
+		!is_same<_UHead, _Head>::value, bool>::type = false>
         constexpr _Tuple_impl(_Tuple_impl<_Idx, _UHead>&& __in)
-	: _Base(std::forward<_UHead>(_Tuple_impl<_Idx, _UHead>::_M_head(__in)))
+	: _Base(__element_arg_t(),
+		std::forward<_UHead>(_Tuple_impl<_Idx, _UHead>::_M_head(__in)))
 	{ }
 
       template<typename _Alloc>
 	_Tuple_impl(allocator_arg_t __tag, const _Alloc& __a)
 	: _Base(__tag, __use_alloc<_Head>(__a)) { }
 
       template<typename _Alloc>
 	_Tuple_impl(allocator_arg_t __tag, const _Alloc& __a,
 		    const _Head& __head)
 	: _Base(__use_alloc<_Head, _Alloc, _Head>(__a), __head) { }
Index: libstdc++-v3/testsuite/20_util/tuple/nomove.cc
===================================================================
--- libstdc++-v3/testsuite/20_util/tuple/nomove.cc	(revision 0)
+++ libstdc++-v3/testsuite/20_util/tuple/nomove.cc	(working copy)
@@ -0,0 +1,39 @@ 
+// { dg-do compile }
+// { dg-options "-std=gnu++11" }
+
+// Copyright (C) 2016 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <tuple>
+#include <type_traits>
+
+struct A { A(A&&)=delete; };
+struct B { int i; B(B&&)=delete; };
+
+static_assert(!std::is_copy_constructible<std::tuple<A>>::value);
+static_assert(!std::is_copy_constructible<std::tuple<B>>::value);
+static_assert(!std::is_copy_constructible<std::tuple<A,int>>::value);
+static_assert(!std::is_copy_constructible<std::tuple<B,int>>::value);
+static_assert(!std::is_copy_constructible<std::tuple<int,A>>::value);
+static_assert(!std::is_copy_constructible<std::tuple<int,B>>::value);
+static_assert(!std::is_move_constructible<std::tuple<A>>::value);
+static_assert(!std::is_move_constructible<std::tuple<B>>::value);
+static_assert(!std::is_move_constructible<std::tuple<A,int>>::value);
+static_assert(!std::is_move_constructible<std::tuple<B,int>>::value);
+static_assert(!std::is_move_constructible<std::tuple<int,A>>::value);
+static_assert(!std::is_move_constructible<std::tuple<int,B>>::value);
+