diff mbox

PR77990 refactor unique_ptr to encapsulate tuple

Message ID 20161017133759.GL2922@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Oct. 17, 2016, 1:37 p.m. UTC 
We are incorrectly requiring unique_ptr deleters to be copyable here:

      explicit
      unique_ptr(pointer __p) noexcept
      : _M_t(__p, deleter_type())
      { }

We could just do:

      explicit
      unique_ptr(pointer __p) noexcept
      : _M_t()
      { std::get<0>(_M_t) = __p; }

But having to deal directly with the std::tuple inside unique_ptr has
been bothering me for some time. The tuple is used so we get the empty
base-class optimisation for the deleter, but that implementation
detail 

This patch refactors unique_ptr to put the std::tuple member into a
new type which provides named accessors for the tuple elements, so we
can stop using get<0> and get<1>. That new type can also provide a
single-argument constructor to fix the copyable requirement for
deleters. This also removes the code for deducing the pointer type
which is duplciated in unique_ptr and unique_ptr<T[], D>, and while in
the neighbourhood I changed it from old-school SFINAE using overloaded
functions to the new hotness with __void_t<>.

I intend to commit this to trunk, but on the branches I'll just fix
the constructor as shown above, as it's a smaller change.


	PR libstdc++/77990
	* include/bits/unique_ptr.h (__uniq_ptr_impl): New type to
	encapsulate implementation details.
	(unique_ptr::unique_ptr(_Up)): Don't copy deleter object.
	(unique_ptr::get, unique_ptr::get_deleter, unique_ptr::release):
	Call member functions of implementation object.
	(unique_ptr<T[], D>): Likewise.
	* python/libstdcxx/v6/printers.py (UniquePointerPrinter): Adjust for
	new implementation.
	* python/libstdcxx/v6/xmethods.py (UniquePtrGetWorker): Likewise.
	* testsuite/20_util/unique_ptr/assign/48635_neg.cc: Adjust dg-error
	lines.
	* testsuite/20_util/unique_ptr/assign/cv_qual.cc: Likewise.
	* testsuite/20_util/unique_ptr/cons/cv_qual.cc: Likewise.
	* testsuite/20_util/unique_ptr/cons/77990.cc: New test.

Tested powerpc64le-linux.

Comments

Jonathan Wakely Oct. 18, 2016, 11:54 a.m. UTC | #1 
On 17/10/16 14:37 +0100, Jonathan Wakely wrote:
>We are incorrectly requiring unique_ptr deleters to be copyable here:
>
>     explicit
>     unique_ptr(pointer __p) noexcept
>     : _M_t(__p, deleter_type())
>     { }
>
>We could just do:
>
>     explicit
>     unique_ptr(pointer __p) noexcept
>     : _M_t()
>     { std::get<0>(_M_t) = __p; }
>
>But having to deal directly with the std::tuple inside unique_ptr has
>been bothering me for some time. The tuple is used so we get the empty
>base-class optimisation for the deleter, but that implementation
>detail

ops, a dangling sentence. I meant to say that the tuple implementation
details leaks into the definition of lots of members, which have to
say std::get<0>(_M_t) or std::get<1>(_M_t) instead of using more
natural member names.

>This patch refactors unique_ptr to put the std::tuple member into a
>new type which provides named accessors for the tuple elements, so we
>can stop using get<0> and get<1>. That new type can also provide a
>single-argument constructor to fix the copyable requirement for
>deleters. This also removes the code for deducing the pointer type
>which is duplciated in unique_ptr and unique_ptr<T[], D>, and while in
>the neighbourhood I changed it from old-school SFINAE using overloaded
>functions to the new hotness with __void_t<>.
>
>I intend to commit this to trunk, but on the branches I'll just fix
>the constructor as shown above, as it's a smaller change.

I'll wait a bit longer for any objections, as the refactoring could be
seen as unnecessary churn, but I think it's valuable housekeeping.
Pedro Alves Oct. 18, 2016, 4:13 p.m. UTC | #2 
On 10/18/2016 12:54 PM, Jonathan Wakely wrote:

> I'll wait a bit longer for any objections, as the refactoring could be
> seen as unnecessary churn, but I think it's valuable housekeeping.

Having stared at std::unique_ptr a lot recently, I like this, FWIW.

Thanks,
Pedro Alves
diff mbox

Patch

commit 48463e9c51e49d0c8972b5128ab8640aa90bc6a3
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Oct 14 22:56:11 2016 +0100

    PR77990 refactor unique_ptr to encapsulate tuple
    
    	PR libstdc++/77990
    	* include/bits/unique_ptr.h (__uniq_ptr_impl): New type to
    	encapsulate implementation details.
    	(unique_ptr::unique_ptr(_Up)): Don't copy deleter object.
    	(unique_ptr::get, unique_ptr::get_deleter, unique_ptr::release):
    	Call member functions of implementation object.
    	(unique_ptr<T[], D>): Likewise.
    	* python/libstdcxx/v6/printers.py (UniquePointerPrinter): Adjust for
    	new implementation.
    	* python/libstdcxx/v6/xmethods.py (UniquePtrGetWorker): Likewise.
    	* testsuite/20_util/unique_ptr/assign/48635_neg.cc: Adjust dg-error
    	lines.
    	* testsuite/20_util/unique_ptr/assign/cv_qual.cc: Likewise.
    	* testsuite/20_util/unique_ptr/cons/cv_qual.cc: Likewise.
    	* testsuite/20_util/unique_ptr/cons/77990.cc: New test.

diff --git a/libstdc++-v3/include/bits/unique_ptr.h b/libstdc++-v3/include/bits/unique_ptr.h
index a36794d..e32b6c9 100644
--- a/libstdc++-v3/include/bits/unique_ptr.h
+++ b/libstdc++-v3/include/bits/unique_ptr.h
@@ -111,33 +111,51 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       }
     };
 
-  /// 20.7.1.2 unique_ptr for single objects.
-  template <typename _Tp, typename _Dp = default_delete<_Tp> >
-    class unique_ptr
+  template <typename _Tp, typename _Dp>
+    class __uniq_ptr_impl
     {
-      // use SFINAE to determine whether _Del::pointer exists
-      class _Pointer
-      {
-	template<typename _Up>
-	  static typename _Up::pointer __test(typename _Up::pointer*);
+      template <typename _Up, typename _Ep, typename = void>
+	struct _Ptr
+	{
+	  using type = _Up*;
+	};
 
-	template<typename _Up>
-	  static _Tp* __test(...);
-
-	typedef typename remove_reference<_Dp>::type _Del;
-
-      public:
-	typedef decltype(__test<_Del>(0)) type;
-      };
-
-      typedef std::tuple<typename _Pointer::type, _Dp>  __tuple_type;
-      __tuple_type                                      _M_t;
+      template <typename _Up, typename _Ep>
+	struct
+	_Ptr<_Up, _Ep, __void_t<typename remove_reference<_Ep>::type::pointer>>
+	{
+	  using type = typename remove_reference<_Ep>::type::pointer;
+	};
 
     public:
-      typedef typename _Pointer::type   pointer;
-      typedef _Tp                       element_type;
-      typedef _Dp                       deleter_type;
+      using pointer = typename _Ptr<_Tp, _Dp>::type;
 
+      __uniq_ptr_impl() = default;
+      __uniq_ptr_impl(pointer __p) : _M_t() { _M_ptr() = __p; }
+
+      template<typename _Del>
+      __uniq_ptr_impl(pointer __p, _Del&& __d)
+	: _M_t(__p, std::forward<_Del>(__d)) { }
+
+      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); }
+
+    private:
+      tuple<pointer, _Dp> _M_t;
+    };
+
+  /// 20.7.1.2 unique_ptr for single objects.
+  template <typename _Tp, typename _Dp = default_delete<_Tp>>
+    class unique_ptr
+    {
+      __uniq_ptr_impl<_Tp, _Dp> _M_t;
+
+    public:
+      using pointer	  = typename __uniq_ptr_impl<_Tp, _Dp>::pointer;
+      using element_type  = _Tp;
+      using deleter_type  = _Dp;
 
       // helper template for detecting a safe conversion from another
       // unique_ptr
@@ -168,7 +186,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
        */
       explicit
       unique_ptr(pointer __p) noexcept
-      : _M_t(__p, deleter_type())
+      : _M_t(__p)
       { static_assert(!is_pointer<deleter_type>::value,
 		     "constructed with null function pointer deleter"); }
 
@@ -231,7 +249,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       /// Destructor, invokes the deleter if the stored pointer is not null.
       ~unique_ptr() noexcept
       {
-	auto& __ptr = std::get<0>(_M_t);
+	auto& __ptr = _M_t._M_ptr();
 	if (__ptr != nullptr)
 	  get_deleter()(__ptr);
 	__ptr = pointer();
@@ -302,17 +320,17 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       /// Return the stored pointer.
       pointer
       get() const noexcept
-      { return std::get<0>(_M_t); }
+      { return _M_t._M_ptr(); }
 
       /// Return a reference to the stored deleter.
       deleter_type&
       get_deleter() noexcept
-      { return std::get<1>(_M_t); }
+      { return _M_t._M_deleter(); }
 
       /// Return a reference to the stored deleter.
       const deleter_type&
       get_deleter() const noexcept
-      { return std::get<1>(_M_t); }
+      { return _M_t._M_deleter(); }
 
       /// Return @c true if the stored pointer is not null.
       explicit operator bool() const noexcept
@@ -325,7 +343,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       release() noexcept
       {
 	pointer __p = get();
-	std::get<0>(_M_t) = pointer();
+	_M_t._M_ptr() = pointer();
 	return __p;
       }
 
@@ -339,7 +357,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       reset(pointer __p = pointer()) noexcept
       {
 	using std::swap;
-	swap(std::get<0>(_M_t), __p);
+	swap(_M_t._M_ptr(), __p);
 	if (__p != pointer())
 	  get_deleter()(__p);
       }
@@ -364,23 +382,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _Tp, typename _Dp>
     class unique_ptr<_Tp[], _Dp>
     {
-      // use SFINAE to determine whether _Del::pointer exists
-      class _Pointer
-      {
-	template<typename _Up>
-	  static typename _Up::pointer __test(typename _Up::pointer*);
-
-	template<typename _Up>
-	  static _Tp* __test(...);
-
-	typedef typename remove_reference<_Dp>::type _Del;
-
-      public:
-	typedef decltype(__test<_Del>(0)) type;
-      };
-
-      typedef std::tuple<typename _Pointer::type, _Dp>  __tuple_type;
-      __tuple_type                                      _M_t;
+      __uniq_ptr_impl<_Tp, _Dp> _M_t;
 
       template<typename _Up>
 	using __remove_cv = typename remove_cv<_Up>::type;
@@ -391,11 +393,10 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  = __and_< is_base_of<_Tp, _Up>,
 		    __not_<is_same<__remove_cv<_Tp>, __remove_cv<_Up>>> >;
 
-
     public:
-      typedef typename _Pointer::type	pointer;
-      typedef _Tp		 	element_type;
-      typedef _Dp                       deleter_type;
+      using pointer	  = typename __uniq_ptr_impl<_Tp, _Dp>::pointer;
+      using element_type  = _Tp;
+      using deleter_type  = _Dp;
 
       // helper template for detecting a safe conversion from another
       // unique_ptr
@@ -446,7 +447,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
                  __safe_conversion_raw<_Up>::value, bool>::type>
       explicit
       unique_ptr(_Up __p) noexcept
-      : _M_t(__p, deleter_type())
+      : _M_t(__p)
       { static_assert(!is_pointer<deleter_type>::value,
 		      "constructed with null function pointer deleter"); }
 
@@ -499,7 +500,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       /// Destructor, invokes the deleter if the stored pointer is not null.
       ~unique_ptr()
       {
-	auto& __ptr = std::get<0>(_M_t);
+	auto& __ptr = _M_t._M_ptr();
 	if (__ptr != nullptr)
 	  get_deleter()(__ptr);
 	__ptr = pointer();
@@ -562,17 +563,17 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       /// Return the stored pointer.
       pointer
       get() const noexcept
-      { return std::get<0>(_M_t); }
+      { return _M_t._M_ptr(); }
 
       /// Return a reference to the stored deleter.
       deleter_type&
       get_deleter() noexcept
-      { return std::get<1>(_M_t); }
+      { return _M_t._M_deleter(); }
 
       /// Return a reference to the stored deleter.
       const deleter_type&
       get_deleter() const noexcept
-      { return std::get<1>(_M_t); }
+      { return _M_t._M_deleter(); }
 
       /// Return @c true if the stored pointer is not null.
       explicit operator bool() const noexcept
@@ -585,7 +586,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       release() noexcept
       {
 	pointer __p = get();
-	std::get<0>(_M_t) = pointer();
+	_M_t._M_ptr() = pointer();
 	return __p;
       }
 
@@ -612,7 +613,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	pointer __ptr = __p;
 	using std::swap;
-	swap(std::get<0>(_M_t), __ptr);
+	swap(_M_t._M_ptr(), __ptr);
 	if (__ptr != nullptr)
 	  get_deleter()(__ptr);
       }
diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index 714e1b8..bad42b4 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -126,7 +126,13 @@  class UniquePointerPrinter:
         self.val = val
 
     def to_string (self):
-        v = self.val['_M_t']['_M_head_impl']
+        impl_type = self.val.type.fields()[0].type.tag
+        if impl_type.startswith('std::__uniq_ptr_impl<'): # New implementation
+            v = self.val['_M_t']['_M_t']['_M_head_impl']
+        elif impl_type.startswith('std::tuple<'):
+            v = self.val['_M_t']['_M_head_impl']
+        else:
+            raise ValueError("Unsupported implementation for unique_ptr: %s" % self.val.type.fields()[0].type.tag)
         return ('std::unique_ptr<%s> containing %s' % (str(v.type.target()),
                                                        str(v)))
 
diff --git a/libstdc++-v3/python/libstdcxx/v6/xmethods.py b/libstdc++-v3/python/libstdcxx/v6/xmethods.py
index 605cda1..045b661 100644
--- a/libstdc++-v3/python/libstdcxx/v6/xmethods.py
+++ b/libstdc++-v3/python/libstdcxx/v6/xmethods.py
@@ -575,7 +575,12 @@  class UniquePtrGetWorker(gdb.xmethod.XMethodWorker):
         return self._elem_type.pointer()
 
     def __call__(self, obj):
-        return obj['_M_t']['_M_head_impl']
+        impl_type = obj.dereference().type.fields()[0].type.tag
+        if impl_type.startswith('std::__uniq_ptr_impl<'): # New implementation
+            return obj['_M_t']['_M_t']['_M_head_impl']
+        elif impl_type.startswith('std::tuple<'):
+            return obj['_M_t']['_M_head_impl']
+        return None
 
 class UniquePtrDerefWorker(UniquePtrGetWorker):
     def __init__(self, elem_type):
diff --git a/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc b/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
index 7f6d27c..311e8d5 100644
--- a/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
+++ b/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
@@ -42,10 +42,10 @@  void f()
   std::unique_ptr<int, D&> ud(nullptr, d);
   ub = std::move(ud); // { dg-error "no match" }
   ub2 = ud; // { dg-error "no match" }
-// { dg-error "no type" "" { target *-*-* } 269 }
+// { dg-error "no type" "" { target *-*-* } 287 }
 
   std::unique_ptr<int[], B&> uba(nullptr, b);
   std::unique_ptr<int[], D&> uda(nullptr, d);
   uba = std::move(uda); // { dg-error "no match" }
-// { dg-error "no type" "" { target *-*-* } 537 }
+// { dg-error "no type" "" { target *-*-* } 538 }
 }
diff --git a/libstdc++-v3/testsuite/20_util/unique_ptr/cons/77990.cc b/libstdc++-v3/testsuite/20_util/unique_ptr/cons/77990.cc
new file mode 100644
index 0000000..e3e9a20
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/unique_ptr/cons/77990.cc
@@ -0,0 +1,27 @@ 
+// 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/>.
+
+// { dg-do compile { target c++11 } }
+
+#include <memory>
+
+struct D {
+  D() = default;
+  D(const D&) = delete;
+  void operator()(int*);
+};
+std::unique_ptr<int, D> p((int*)nullptr);  // PR libstdc++/77990
diff --git a/libstdc++-v3/testsuite/20_util/unique_ptr/cons/cv_qual.cc b/libstdc++-v3/testsuite/20_util/unique_ptr/cons/cv_qual.cc
index fcd5c5e..829d112 100644
--- a/libstdc++-v3/testsuite/20_util/unique_ptr/cons/cv_qual.cc
+++ b/libstdc++-v3/testsuite/20_util/unique_ptr/cons/cv_qual.cc
@@ -105,7 +105,7 @@  test07()
   std::unique_ptr<const A[]> cA3(p); // { dg-error "no matching function" }
   std::unique_ptr<volatile A[]> vA3(p); // { dg-error "no matching function" }
   std::unique_ptr<const volatile A[]> cvA3(p); // { dg-error "no matching function" }
-  // { dg-error "no type" "" { target *-*-* } 445 }
+  // { dg-error "no type" "" { target *-*-* } 446 }
 }
 
 template<typename T>