From patchwork Mon Dec 12 16:11:33 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 87715 Delivered-To: patch@linaro.org Received: by 10.182.112.6 with SMTP id im6csp1728199obb; Mon, 12 Dec 2016 08:12:04 -0800 (PST) X-Received: by 10.98.194.86 with SMTP id l83mr96454445pfg.64.1481559124581; Mon, 12 Dec 2016 08:12:04 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id 206si43977144pfw.94.2016.12.12.08.12.04 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 12 Dec 2016 08:12:04 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-444180-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-444180-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-444180-patch=linaro.org@gcc.gnu.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type :content-transfer-encoding; q=dns; s=default; b=sVnczQNIiT3L4/UD 1bvLahiZktuc3OtUd/Mq6SakbAnAg6PYVTg0kEn1yqbHQXqY49Tupf+owthQ/B+H r/SDZ/QNYxnRl2kjzUFoH+9cU8h+wCn9d5oOwKmYQACQwZn3/8A4yfmSZIzHtJmW ET+qix5dw5NQcqN8PBeGdw+mqDA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type :content-transfer-encoding; s=default; bh=EiZ46yk10CkuCDFCZgskpv ccM8A=; b=hhagRsNW45pRwendzbdRwFIHGMKRsLk21xr6yfLsqKdE+wZetuGdy0 2puGUO1mchjOpJVQaj4KBNVQx5GSOun+2E4n6mo+Pprm/Fpiua7VInQ58MK9sic/ LcRwS0Nj4t25fSdLtjEyBaUOfxLxs+X0OGs9BXb3ck4Vix7nZn/tA= Received: (qmail 66075 invoked by alias); 12 Dec 2016 16:11:48 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 65840 invoked by uid 89); 12 Dec 2016 16:11:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.5 required=5.0 tests=BAYES_00, FAKE_REPLY_C, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=const_cast, 5577, Found, noisily X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 12 Dec 2016 16:11:37 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1D805624AA; Mon, 12 Dec 2016 16:11:36 +0000 (UTC) Received: from localhost (ovpn-116-115.ams2.redhat.com [10.36.116.115]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uBCGBXZ7011917; Mon, 12 Dec 2016 11:11:34 -0500 Date: Mon, 12 Dec 2016 16:11:33 +0000 From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Make std::enable_shared_from_this cope with ambiguity Message-ID: <20161212161133.GL6326@redhat.com> MIME-Version: 1.0 Content-Disposition: inline X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.7.1 (2016-10-04) 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. commit 8aee705324a8a5463a89a6cab0d77c20ae99ee41 Author: Jonathan Wakely Date: Mon Dec 12 15:25:14 2016 +0000 Enable experimental::enable_shared_from_this explicitly Backport from mainline 2016-10-19 Jonathan Wakely * 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. 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> explicit - shared_ptr(_Tp1* __p) : _Base_type(__p) { } + shared_ptr(_Tp1* __p) : _Base_type(__p) + { _M_enable_shared_from_this_with(__p); } template> shared_ptr(_Tp1* __p, _Deleter __d) - : _Base_type(__p, __d) { } + : _Base_type(__p, __d) + { _M_enable_shared_from_this_with(__p); } template> 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 shared_ptr(nullptr_t __p, _Deleter __d) @@ -785,13 +786,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #if _GLIBCXX_USE_DEPRECATED template> 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> 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 friend shared_ptr<_Tp1> @@ -863,6 +871,35 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION : _Base_type(__r, std::nothrow) { } friend class weak_ptr<_Tp>; + + template + 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 + struct __has_esft_base + : false_type { }; + + template + struct __has_esft_base<_Yp, __void_t<__esft_base_t<_Yp>>> + : __bool_constant> { }; // ignore base for arrays + + template + 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 enable_if::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 - 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 + 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 +// . + +// { dg-do run { target c++14 } } + +#include +#include + +struct A : std::enable_shared_from_this { }; +struct B : std::experimental::enable_shared_from_this { }; +struct C : A, B { }; + +void +test01() +{ + // This should not fail to compile due to ambiguous base classes: + std::experimental::shared_ptr p(new C); + + // And both base classes should have been enabled: + std::shared_ptr pa = p->A::shared_from_this(); + VERIFY( pa != nullptr ); + // Can't compare pa and p because they're different types + + std::experimental::shared_ptr 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 up(new A); std::experimental::shared_ptr 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 );