This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: tuple move constructor


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.

--
Marc Glisse
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);
+

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]