LWG 2899 - Make is_move_constructible correct for unique_ptr

Message ID 20190514111752.GA32214@redhat.com
State New
Headers show
Series
  • LWG 2899 - Make is_move_constructible correct for unique_ptr
Related show

Commit Message

Jonathan Wakely May 14, 2019, 11:17 a.m.
* include/bits/unique_ptr.h (__uniq_ptr_impl): Add move constructor,
	move assignment operator.
	(__uniq_ptr_impl::release(), __uniq_ptr_impl::reset(pointer)): Add.
	(__uniq_ptr_data): New class template with conditionally deleted
	special members.
	(unique_ptr, unique_ptr<T[], D>): Change type of data member from
	__uniq_ptr_impl<T, D> to __uniq_ptr_data<T, D>. Define move
	constructor and move assignment operator as defaulted.
	(unique_ptr::release(), unique_ptr<T[], D>::release()): Forward to
	__uniq_ptr_impl::release().
	(unique_ptr::reset(pointer), unique_ptr<T[], D>::reset<U>(U)): Forward
	to __uniq_ptr_impl::reset(pointer).
	* python/libstdcxx/v6/printers.py (UniquePointerPrinter.__init__):
	Check for new __uniq_ptr_data type.
	* testsuite/20_util/unique_ptr/dr2899.cc: New test.

Tested powerpc64le-linux, committed to trunk.
commit a136419c054eeaa2e88246d682151c25c124d30e
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Feb 14 17:00:20 2019 +0000

    LWG 2899 - Make is_move_constructible correct for unique_ptr
    
            * include/bits/unique_ptr.h (__uniq_ptr_impl): Add move constructor,
            move assignment operator.
            (__uniq_ptr_impl::release(), __uniq_ptr_impl::reset(pointer)): Add.
            (__uniq_ptr_data): New class template with conditionally deleted
            special members.
            (unique_ptr, unique_ptr<T[], D>): Change type of data member from
            __uniq_ptr_impl<T, D> to __uniq_ptr_data<T, D>. Define move
            constructor and move assignment operator as defaulted.
            (unique_ptr::release(), unique_ptr<T[], D>::release()): Forward to
            __uniq_ptr_impl::release().
            (unique_ptr::reset(pointer), unique_ptr<T[], D>::reset<U>(U)): Forward
            to __uniq_ptr_impl::reset(pointer).
            * python/libstdcxx/v6/printers.py (UniquePointerPrinter.__init__):
            Check for new __uniq_ptr_data type.
            * testsuite/20_util/unique_ptr/dr2899.cc: New test.

Comments

Jonathan Wakely May 17, 2019, 11:07 p.m. | #1
On 14/05/19 12:17 +0100, Jonathan Wakely wrote:
>	* include/bits/unique_ptr.h (__uniq_ptr_impl): Add move constructor,

>	move assignment operator.

>	(__uniq_ptr_impl::release(), __uniq_ptr_impl::reset(pointer)): Add.

>	(__uniq_ptr_data): New class template with conditionally deleted

>	special members.

>	(unique_ptr, unique_ptr<T[], D>): Change type of data member from

>	__uniq_ptr_impl<T, D> to __uniq_ptr_data<T, D>. Define move

>	constructor and move assignment operator as defaulted.

>	(unique_ptr::release(), unique_ptr<T[], D>::release()): Forward to

>	__uniq_ptr_impl::release().

>	(unique_ptr::reset(pointer), unique_ptr<T[], D>::reset<U>(U)): Forward

>	to __uniq_ptr_impl::reset(pointer).

>	* python/libstdcxx/v6/printers.py (UniquePointerPrinter.__init__):

>	Check for new __uniq_ptr_data type.

>	* testsuite/20_util/unique_ptr/dr2899.cc: New test.


This updates the Xmethod for the new base class.

Tested x86_64-linux, committed to trunk.
commit 5b013bcaeae51850ced6eaff0c6626915565c88a
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Sat May 18 00:05:47 2019 +0100

    PR libstdc++/90520 adjust Xmethod for recent unique_ptr changes
    
            PR libstdc++/90520
            * python/libstdcxx/v6/printers.py (UniquePointerPrinter.__init__):
            Raise exception if unique_ptr tuple member has unknown structure.
            * python/libstdcxx/v6/xmethods.py (UniquePtrGetWorker.__call__):
            Adjust worker to support new __uniq_ptr_data base class. Do not
            assume field called _M_head_impl is the first tuple element.

diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index 162b00760e6..05143153bee 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -197,6 +197,8 @@ class UniquePointerPrinter:
             self.pointer = tuple_member['_M_head_impl']
         elif head_field.is_base_class:
             self.pointer = tuple_member.cast(head_field.type)
+        else:
+            raise ValueError("Unsupported implementation for tuple in unique_ptr: %s" % impl_type)
 
     def children (self):
         return SmartPtrIterator(self.pointer)
diff --git a/libstdc++-v3/python/libstdcxx/v6/xmethods.py b/libstdc++-v3/python/libstdcxx/v6/xmethods.py
index c405d8a25d5..623cb80bc0e 100644
--- a/libstdc++-v3/python/libstdcxx/v6/xmethods.py
+++ b/libstdc++-v3/python/libstdcxx/v6/xmethods.py
@@ -586,11 +586,22 @@ class UniquePtrGetWorker(gdb.xmethod.XMethodWorker):
 
     def __call__(self, obj):
         impl_type = obj.dereference().type.fields()[0].type.tag
-        if re.match('^std::(__\d+::)?__uniq_ptr_impl<.*>$', impl_type): # New implementation
-            return obj['_M_t']['_M_t']['_M_head_impl']
+        # Check for new implementations first:
+        if re.match('^std::(__\d+::)?__uniq_ptr_(data|impl)<.*>$', impl_type):
+            tuple_member = obj['_M_t']['_M_t']
         elif re.match('^std::(__\d+::)?tuple<.*>$', impl_type):
-            return obj['_M_t']['_M_head_impl']
-        return None
+            tuple_member = obj['_M_t']
+        else:
+            return None
+        tuple_impl_type = tuple_member.type.fields()[0].type # _Tuple_impl
+        tuple_head_type = tuple_impl_type.fields()[1].type   # _Head_base
+        head_field = tuple_head_type.fields()[0]
+        if head_field.name == '_M_head_impl':
+            return tuple_member['_M_head_impl']
+        elif head_field.is_base_class:
+            return tuple_member.cast(head_field.type)
+        else:
+            return None
 
 class UniquePtrDerefWorker(UniquePtrGetWorker):
     "Implements std::unique_ptr<T>::operator*()"

Patch

diff --git a/libstdc++-v3/include/bits/unique_ptr.h b/libstdc++-v3/include/bits/unique_ptr.h
index a9e74725dfd..484c8b328e4 100644
--- a/libstdc++-v3/include/bits/unique_ptr.h
+++ b/libstdc++-v3/include/bits/unique_ptr.h
@@ -119,6 +119,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   /// @cond undocumented
 
+  // Manages the pointer and deleter of a unique_ptr
   template <typename _Tp, typename _Dp>
     class __uniq_ptr_impl
     {
@@ -153,14 +154,75 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       __uniq_ptr_impl(pointer __p, _Del&& __d)
 	: _M_t(__p, std::forward<_Del>(__d)) { }
 
+      __uniq_ptr_impl(__uniq_ptr_impl&& __u) noexcept
+      : _M_t(std::move(__u._M_t))
+      { __u._M_ptr() = nullptr; }
+
+      __uniq_ptr_impl& operator=(__uniq_ptr_impl&& __u) noexcept
+      {
+	reset(__u.release());
+	_M_deleter() = std::forward<_Dp>(__u._M_deleter());
+	return *this;
+      }
+
       pointer&   _M_ptr() { return std::get<0>(_M_t); }
       pointer    _M_ptr() const { return std::get<0>(_M_t); }
       _Dp&       _M_deleter() { return std::get<1>(_M_t); }
       const _Dp& _M_deleter() const { return std::get<1>(_M_t); }
 
+      void reset(pointer __p) noexcept
+      {
+	const pointer __old_p = _M_ptr();
+	_M_ptr() = __p;
+	if (__old_p)
+	  _M_deleter()(__old_p);
+      }
+
+      pointer release() noexcept
+      {
+	pointer __p = _M_ptr();
+	_M_ptr() = nullptr;
+	return __p;
+      }
+
     private:
       tuple<pointer, _Dp> _M_t;
     };
+
+  // Defines move construction + assignment as either defaulted or deleted.
+  template <typename _Tp, typename _Dp,
+	    bool = is_move_constructible<_Dp>::value,
+	    bool = is_move_assignable<_Dp>::value>
+    struct __uniq_ptr_data : __uniq_ptr_impl<_Tp, _Dp>
+    {
+      using __uniq_ptr_impl<_Tp, _Dp>::__uniq_ptr_impl;
+      __uniq_ptr_data(__uniq_ptr_data&&) = default;
+      __uniq_ptr_data& operator=(__uniq_ptr_data&&) = default;
+    };
+
+  template <typename _Tp, typename _Dp>
+    struct __uniq_ptr_data<_Tp, _Dp, true, false> : __uniq_ptr_impl<_Tp, _Dp>
+    {
+      using __uniq_ptr_impl<_Tp, _Dp>::__uniq_ptr_impl;
+      __uniq_ptr_data(__uniq_ptr_data&&) = default;
+      __uniq_ptr_data& operator=(__uniq_ptr_data&&) = delete;
+    };
+
+  template <typename _Tp, typename _Dp>
+    struct __uniq_ptr_data<_Tp, _Dp, false, true> : __uniq_ptr_impl<_Tp, _Dp>
+    {
+      using __uniq_ptr_impl<_Tp, _Dp>::__uniq_ptr_impl;
+      __uniq_ptr_data(__uniq_ptr_data&&) = delete;
+      __uniq_ptr_data& operator=(__uniq_ptr_data&&) = default;
+    };
+
+  template <typename _Tp, typename _Dp>
+    struct __uniq_ptr_data<_Tp, _Dp, false, false> : __uniq_ptr_impl<_Tp, _Dp>
+    {
+      using __uniq_ptr_impl<_Tp, _Dp>::__uniq_ptr_impl;
+      __uniq_ptr_data(__uniq_ptr_data&&) = delete;
+      __uniq_ptr_data& operator=(__uniq_ptr_data&&) = delete;
+    };
   /// @endcond
 
   /// 20.7.1.2 unique_ptr for single objects.
@@ -171,7 +233,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	using _DeleterConstraint =
 	  typename __uniq_ptr_impl<_Tp, _Up>::_DeleterConstraint::type;
 
-      __uniq_ptr_impl<_Tp, _Dp> _M_t;
+      __uniq_ptr_data<_Tp, _Dp> _M_t;
 
     public:
       using pointer	  = typename __uniq_ptr_impl<_Tp, _Dp>::pointer;
@@ -255,8 +317,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // Move constructors.
 
       /// Move constructor.
-      unique_ptr(unique_ptr&& __u) noexcept
-      : _M_t(__u.release(), std::forward<deleter_type>(__u.get_deleter())) { }
+      unique_ptr(unique_ptr&&) = default;
 
       /** @brief Converting constructor from another type
        *
@@ -298,24 +359,16 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       /** @brief Move assignment operator.
        *
-       * @param __u  The object to transfer ownership from.
-       *
-       * Invokes the deleter first if this object owns a pointer.
+       * Invokes the deleter if this object owns a pointer.
        */
-      unique_ptr&
-      operator=(unique_ptr&& __u) noexcept
-      {
-	reset(__u.release());
-	get_deleter() = std::forward<deleter_type>(__u.get_deleter());
-	return *this;
-      }
+      unique_ptr& operator=(unique_ptr&&) = default;
 
       /** @brief Assignment from another type.
        *
        * @param __u  The object to transfer ownership from, which owns a
        *             convertible pointer to a non-array object.
        *
-       * Invokes the deleter first if this object owns a pointer.
+       * Invokes the deleter if this object owns a pointer.
        */
       template<typename _Up, typename _Ep>
         typename enable_if< __and_<
@@ -380,11 +433,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       /// Release ownership of any stored pointer.
       pointer
       release() noexcept
-      {
-	pointer __p = get();
-	_M_t._M_ptr() = pointer();
-	return __p;
-      }
+      { return _M_t.release(); }
 
       /** @brief Replace the stored pointer.
        *
@@ -397,10 +446,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	static_assert(__is_invocable<deleter_type&, pointer>::value,
 		      "unique_ptr's deleter must be invocable with a pointer");
-	using std::swap;
-	swap(_M_t._M_ptr(), __p);
-	if (__p != pointer())
-	  get_deleter()(std::move(__p));
+	_M_t.reset(std::move(__p));
       }
 
       /// Exchange the pointer and deleter with another object.
@@ -427,7 +473,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       using _DeleterConstraint =
 	typename __uniq_ptr_impl<_Tp, _Up>::_DeleterConstraint::type;
 
-      __uniq_ptr_impl<_Tp, _Dp> _M_t;
+      __uniq_ptr_data<_Tp, _Dp> _M_t;
 
       template<typename _Up>
 	using __remove_cv = typename remove_cv<_Up>::type;
@@ -536,8 +582,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 				 _DelUnref&&>) = delete;
 
       /// Move constructor.
-      unique_ptr(unique_ptr&& __u) noexcept
-      : _M_t(__u.release(), std::forward<deleter_type>(__u.get_deleter())) { }
+      unique_ptr(unique_ptr&&) = default;
 
       /// Creates a unique_ptr that owns nothing.
       template<typename _Del = _Dp, typename = _DeleterConstraint<_Del>>
@@ -564,24 +609,17 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       /** @brief Move assignment operator.
        *
-       * @param __u  The object to transfer ownership from.
-       *
-       * Invokes the deleter first if this object owns a pointer.
+       * Invokes the deleter if this object owns a pointer.
        */
       unique_ptr&
-      operator=(unique_ptr&& __u) noexcept
-      {
-	reset(__u.release());
-	get_deleter() = std::forward<deleter_type>(__u.get_deleter());
-	return *this;
-      }
+      operator=(unique_ptr&&) = default;
 
       /** @brief Assignment from another type.
        *
        * @param __u  The object to transfer ownership from, which owns a
        *             convertible pointer to an array object.
        *
-       * Invokes the deleter first if this object owns a pointer.
+       * Invokes the deleter if this object owns a pointer.
        */
       template<typename _Up, typename _Ep>
 	typename
@@ -638,11 +676,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       /// Release ownership of any stored pointer.
       pointer
       release() noexcept
-      {
-	pointer __p = get();
-	_M_t._M_ptr() = pointer();
-	return __p;
-      }
+      { return _M_t.release(); }
 
       /** @brief Replace the stored pointer.
        *
@@ -664,18 +698,10 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
                >>
       void
       reset(_Up __p) noexcept
-      {
-	pointer __ptr = __p;
-	using std::swap;
-	swap(_M_t._M_ptr(), __ptr);
-	if (__ptr != nullptr)
-	  get_deleter()(__ptr);
-      }
+      { _M_t.reset(std::move(__p)); }
 
       void reset(nullptr_t = nullptr) noexcept
-      {
-        reset(pointer());
-      }
+      { reset(pointer()); }
 
       /// Exchange the pointer and deleter with another object.
       void
diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index 19d367295df..eae06f93c34 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -182,7 +182,9 @@  class UniquePointerPrinter:
     def __init__ (self, typename, val):
         self.val = val
         impl_type = val.type.fields()[0].type.tag
-        if is_specialization_of(impl_type, '__uniq_ptr_impl'): # New implementation
+        # Check for new implementations first:
+        if is_specialization_of(impl_type, '__uniq_ptr_data') \
+            or is_specialization_of(impl_type, '__uniq_ptr_impl'):
             self.pointer = val['_M_t']['_M_t']['_M_head_impl']
         elif is_specialization_of(impl_type, 'tuple'):
             self.pointer = val['_M_t']['_M_head_impl']
diff --git a/libstdc++-v3/testsuite/20_util/unique_ptr/dr2899.cc b/libstdc++-v3/testsuite/20_util/unique_ptr/dr2899.cc
new file mode 100644
index 00000000000..b528841c862
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/unique_ptr/dr2899.cc
@@ -0,0 +1,54 @@ 
+// Copyright (C) 2019 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/>.
+
+// { dg-do compile { target c++11 } }
+
+#include <memory>
+
+struct Del {
+  Del() = default;
+  Del(Del&&) = delete;
+
+  void operator()(int*) const;
+};
+
+static_assert(!std::is_move_constructible<std::unique_ptr<int, Del>>::value);
+static_assert(std::is_move_constructible<std::unique_ptr<int, Del&>>::value);
+
+struct Del2 {
+  Del2() = default;
+  Del2(Del2&&) = default;
+  Del2& operator=(Del2&&) = delete;
+  Del2& operator=(const Del2&) = default;
+
+  void operator()(int*) const;
+};
+
+static_assert(!std::is_move_assignable<std::unique_ptr<int, Del2>>::value);
+static_assert(std::is_move_assignable<std::unique_ptr<int, Del2&>>::value);
+
+struct Del3 {
+  Del3() = default;
+  Del3(Del3&&) = default;
+  Del3& operator=(Del3&&) = default;
+  Del3& operator=(const Del3&) = delete;
+
+  void operator()(int*) const;
+};
+
+static_assert(std::is_move_assignable<std::unique_ptr<int, Del3>>::value);
+static_assert(!std::is_move_assignable<std::unique_ptr<int, Del3&>>::value);