diff mbox

Make std::enable_shared_from_this cope with ambiguity

Message ID 20161212161133.GL6326@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Dec. 12, 2016, 4:11 p.m. UTC
On 19/10/16 21:13 +0100, Jonathan Wakely wrote:
>> This patch does three things:
>> 
>> 1. Refactor std::enable_shared_from_this support code.
>> 
>> Instead of several overloads of __enable_shared_from_this_helper
>> that contain the same code but operating on slightly different types
>> I've split it into two parts. Upcasting to an accessible+unambiguous
>> enable_shared_from_this base class is done by new functions found by
>> ADL, __enable_shared_from_this_base. That is called by a new
>> template function __shared_ptr::_M_enable_shared_from_this_with()
>> which actually does the enabling by calling _M_weak_assign.
>> 
>> Calls to _M_enable_shared_from_this_with(p) closely match the
>> wording from my https://wg21.link/p0033r1 paper ("enables
>> shared_from_this with p") and are constrained so they don't give an
>> error if the __enable_shared_from_this_base() call is ambiguous. We
>> already gracefully handled ambiguous std::enable_shared_from_this
>> bases (PR56383) but failed noisily if a type had a combination of
>> std::enable_shared_from_this, std::__enable_shared_from_this and
>> std::experimental::enable_shared_from_this bases. Now we treat those
>> combinations gracefully.
>> 
>> 2. Change std::experimental::enable_shared_from_this bases to be
>> initialized independently of std::enable_shared_from_this bases.
>> It's now possible to have both, and for both to be enabled. See
>> enable_shared_from_this.cc which tests this.
>> 
>> The standard says we have to enable shared_from_this for types with
>> an accessible and unambiguous std::enable_shared_from_this base
>> class, and we should be able to do that even if the class also has
>> an experimental::enable_shared_from_this base class. Now we can.
>> 
>> Potentially we could do the same for std::__enable_shared_from_this,
>> but I'm happy for those bases to be considered ambiguous with
>> std::enable_shared_from_this.
>> 
>> 3. Don't enable shared_from_this for arrays stored in
>> experimental::shared_ptr.  This matches the boost::shared_ptr
>> semantics, and I intend to propose this as a fix for C++17 too. It
>> doesn't make sense to treat the first element of an array specially
>> and enable shared_from_this for the first element only.
>
>I forgot to say that 2. and 3. are appropriate for gcc-6-branch too,
>as they only touch the experimental TS code.

This adds only that part to gcc-6-branch, so that we don't get
ambiguities between std::enable_shared_from_this base-classes and
experimental::enable_shared_from_this base-class

Tested x86_64-linux, committed to gcc-6-branch.
diff mbox

Patch

commit 8aee705324a8a5463a89a6cab0d77c20ae99ee41
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Dec 12 15:25:14 2016 +0000

    Enable experimental::enable_shared_from_this explicitly
    
    Backport from mainline
    2016-10-19  Jonathan Wakely  <jwakely@redhat.com>
    
    	* include/experimental/bits/shared_ptr.h (experimental::shared_ptr):
    	Change relevant constructors to call _M_enable_shared_from_this_with.
    	(experimental::shared_ptr::__efst_base_t)
    	(experimental::shared_ptr::__has_efst_base): Helpers to detect
    	accessible and unambiguous enable_shared_from_this bases.
    	(experimental::shared_ptr::_M_enable_shared_from_this_with): Define.
    	(experimental::__enable_shared_from_this_helper): Remove overload for
    	std::experimental::enable_shared_from_this.
    	(experimental::__expt_enable_shared_from_this_base): Define friend
    	function to select a std::experimental::enable_shared_from_this base.
    	* testsuite/experimental/memory/shared_ptr/cons/
    	enable_shared_from_this.cc: New test.
    	* testsuite/experimental/memory/shared_ptr/cons/unique_ptr_ctor.cc:
    	Adjust expected behaviour for shared_ptr<A[]>.

diff --git a/libstdc++-v3/include/experimental/bits/shared_ptr.h b/libstdc++-v3/include/experimental/bits/shared_ptr.h
index 2e3da62..e8c533e 100644
--- a/libstdc++-v3/include/experimental/bits/shared_ptr.h
+++ b/libstdc++-v3/include/experimental/bits/shared_ptr.h
@@ -259,7 +259,6 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       //	{
       //	  void* __p = _M_refcount._M_get_deleter(typeid(__tag));
       //	  _M_ptr = static_cast<_Tp*>(__p);
-      //	  __enable_shared_from_this_helper(_M_refcount, _M_ptr, _M_ptr);
       //	}
 
       // __weak_ptr::lock()
@@ -557,7 +556,6 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       //	{
       //	  void* __p = _M_refcount._M_get_deleter(typeid(__tag));
       //	  _M_ptr = static_cast<_Tp*>(__p);
-      //	  __enable_shared_from_this_helper(_M_refcount, _M_ptr, _M_ptr);
       //	}
 
       // __weak_ptr::lock()
@@ -740,16 +738,19 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       template<typename _Tp1, typename = _SafeConv<_Tp1>>
 	explicit
-	shared_ptr(_Tp1* __p) : _Base_type(__p) { }
+	shared_ptr(_Tp1* __p) : _Base_type(__p)
+	{ _M_enable_shared_from_this_with(__p); }
 
       template<typename _Tp1, typename _Deleter, typename = _SafeConv<_Tp1>>
 	shared_ptr(_Tp1* __p, _Deleter __d)
-	: _Base_type(__p, __d) { }
+	: _Base_type(__p, __d)
+	{ _M_enable_shared_from_this_with(__p); }
 
       template<typename _Tp1, typename _Deleter, typename _Alloc,
 	       typename = _SafeConv<_Tp1>>
 	shared_ptr(_Tp1* __p, _Deleter __d, _Alloc __a)
-	: _Base_type(__p, __d, __a) { }
+	: _Base_type(__p, __d, __a)
+	{ _M_enable_shared_from_this_with(__p); }
 
       template<typename _Deleter>
 	shared_ptr(nullptr_t __p, _Deleter __d)
@@ -785,13 +786,20 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #if _GLIBCXX_USE_DEPRECATED
       template<typename _Tp1, typename = _Compatible<_Tp1>>
 	shared_ptr(std::auto_ptr<_Tp1>&& __r)
-	: _Base_type(std::move(__r)) { }
+	: _Base_type(std::move(__r))
+	{ _M_enable_shared_from_this_with(static_cast<_Tp1*>(this->get())); }
 #endif
 
       template<typename _Tp1, typename _Del,
 	       typename = _UniqCompatible<_Tp1, _Del>>
 	shared_ptr(unique_ptr<_Tp1, _Del>&& __r)
-	: _Base_type(std::move(__r)) { }
+	: _Base_type(std::move(__r))
+	{
+	  // XXX assume conversion from __r.get() to this->get() to __elem_t*
+	  // is a round trip, which might not be true in all cases.
+	  using __elem_t = typename unique_ptr<_Tp1, _Del>::element_type;
+	  _M_enable_shared_from_this_with(static_cast<__elem_t*>(this->get()));
+	}
 
       constexpr shared_ptr(nullptr_t __p)
       : _Base_type(__p) { }
@@ -853,7 +861,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	shared_ptr(_Sp_make_shared_tag __tag, const _Alloc& __a,
 		   _Args&&... __args)
 	: _Base_type(__tag, __a, std::forward<_Args>(__args)...)
-	{ }
+	{ _M_enable_shared_from_this_with(this->get()); }
 
       template<typename _Tp1, typename _Alloc, typename... _Args>
 	friend shared_ptr<_Tp1>
@@ -863,6 +871,35 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       : _Base_type(__r, std::nothrow) { }
 
       friend class weak_ptr<_Tp>;
+
+      template<typename _Yp>
+	using __esft_base_t =
+	  decltype(__expt_enable_shared_from_this_base(std::declval<_Yp*>()));
+
+      // Detect an accessible and unambiguous enable_shared_from_this base.
+      template<typename _Yp, typename = void>
+	struct __has_esft_base
+	: false_type { };
+
+      template<typename _Yp>
+	struct __has_esft_base<_Yp, __void_t<__esft_base_t<_Yp>>>
+	: __bool_constant<!is_array_v<_Tp>> { };  // ignore base for arrays
+
+      template<typename _Yp>
+	typename enable_if<__has_esft_base<_Yp>::value>::type
+	_M_enable_shared_from_this_with(const _Yp* __p) noexcept
+	{
+	  if (auto __base = __expt_enable_shared_from_this_base(__p))
+	    {
+	      __base->_M_weak_this
+		= shared_ptr<_Yp>(*this, const_cast<_Yp*>(__p));
+	    }
+	}
+
+      template<typename _Yp>
+	typename enable_if<!__has_esft_base<_Yp>::value>::type
+	_M_enable_shared_from_this_with(const _Yp*) noexcept
+	{ }
     };
 
   // C++14 ??20.8.2.2.7 //DOING
@@ -1258,15 +1295,13 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	_M_weak_assign(_Tp1* __p, const __shared_count<>& __n) const noexcept
 	{ _M_weak_this._M_assign(__p, __n); }
 
-      template<typename _Tp1>
-	friend void
-	__enable_shared_from_this_helper(const __shared_count<>& __pn,
-					 const enable_shared_from_this* __pe,
-					 const _Tp1* __px) noexcept
-	{
-	  if(__pe != 0)
-	    __pe->_M_weak_assign(const_cast<_Tp1*>(__px), __pn);
-	}
+      // Found by ADL when this is an associated class.
+      friend const enable_shared_from_this*
+      __expt_enable_shared_from_this_base(const enable_shared_from_this* __p)
+      { return __p; }
+
+      template<typename>
+	friend class shared_ptr;
 
       mutable weak_ptr<_Tp> _M_weak_this;
     };
diff --git a/libstdc++-v3/testsuite/experimental/memory/shared_ptr/cons/enable_shared_from_this.cc b/libstdc++-v3/testsuite/experimental/memory/shared_ptr/cons/enable_shared_from_this.cc
new file mode 100644
index 0000000..5374f75
--- /dev/null
+++ b/libstdc++-v3/testsuite/experimental/memory/shared_ptr/cons/enable_shared_from_this.cc
@@ -0,0 +1,47 @@ 
+// 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 run { target c++14 } }
+
+#include <experimental/memory>
+#include <testsuite_hooks.h>
+
+struct A : std::enable_shared_from_this<A> { };
+struct B : std::experimental::enable_shared_from_this<B> { };
+struct C : A, B { };
+
+void
+test01()
+{
+  // This should not fail to compile due to ambiguous base classes:
+  std::experimental::shared_ptr<C> p(new C);
+
+  // And both base classes should have been enabled:
+  std::shared_ptr<A> pa = p->A::shared_from_this();
+  VERIFY( pa != nullptr );
+  // Can't compare pa and p because they're different types
+
+  std::experimental::shared_ptr<B> pb = p->B::shared_from_this();
+  VERIFY( pb != nullptr );
+  VERIFY( pb == p );
+}
+
+int
+main()
+{
+  test01();
+}
diff --git a/libstdc++-v3/testsuite/experimental/memory/shared_ptr/cons/unique_ptr_ctor.cc b/libstdc++-v3/testsuite/experimental/memory/shared_ptr/cons/unique_ptr_ctor.cc
index 36b21ce..eb24176 100644
--- a/libstdc++-v3/testsuite/experimental/memory/shared_ptr/cons/unique_ptr_ctor.cc
+++ b/libstdc++-v3/testsuite/experimental/memory/shared_ptr/cons/unique_ptr_ctor.cc
@@ -1,4 +1,4 @@ 
-// { dg-options "-std=gnu++14" }
+// { dg-do run { target c++14 } }
 
 // Copyright (C) 2015-2016 Free Software Foundation, Inc.
 //
@@ -61,7 +61,6 @@  static_assert( !constructible< B[],  A[1] >(), "B[2] -> A[1] not compatible" );
 void
 test01()
 {
-  bool test __attribute__((unused)) = true;
   std::unique_ptr<A> up(new A);
   std::experimental::shared_ptr<A> sp(std::move(up));
   VERIFY( up.get() == 0 );
@@ -84,7 +83,16 @@  test02()
   VERIFY( sp.get() != 0 );
   VERIFY( sp.use_count() == 1 );
 
-  VERIFY( sp[0].shared_from_this() != nullptr );
+  bool caught = false;
+  try
+  {
+    sp[0].shared_from_this(); // should not be set for arrays
+  }
+  catch (const std::bad_weak_ptr&)
+  {
+    caught = true;
+  }
+  VERIFY( caught );
 
   sp.reset();
   VERIFY( destroyed == 5 );