[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL303874: Add support for shared_ptr (authored 
by epilk).

Changed prior to commit:
  https://reviews.llvm.org/D30837?vs=97313=100251#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D30837

Files:
  libcxx/trunk/include/memory
  
libcxx/trunk/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
  
libcxx/trunk/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp

Index: libcxx/trunk/include/memory
===
--- libcxx/trunk/include/memory
+++ libcxx/trunk/include/memory
@@ -2251,6 +2251,8 @@
 
 template 
 struct _LIBCPP_TEMPLATE_VIS default_delete {
+static_assert(!is_function<_Tp>::value,
+  "default_delete cannot be instantiated for function types");
 #ifndef _LIBCPP_CXX03_LANG
   _LIBCPP_INLINE_VISIBILITY constexpr default_delete() noexcept = default;
 #else
@@ -3653,6 +3655,18 @@
 __a.deallocate(_PTraits::pointer_to(*this), 1);
 }
 
+struct __shared_ptr_dummy_rebind_allocator_type;
+template <>
+class _LIBCPP_TEMPLATE_VIS allocator<__shared_ptr_dummy_rebind_allocator_type>
+{
+public:
+template 
+struct rebind
+{
+typedef allocator<_Other> other;
+};
+};
+
 template class _LIBCPP_TEMPLATE_VIS enable_shared_from_this;
 
 template
@@ -3921,6 +3935,17 @@
 #endif  // _LIBCPP_HAS_NO_VARIADICS
 
 private:
+template ::value>
+struct __shared_ptr_default_allocator
+{
+typedef allocator<_Yp> type;
+};
+
+template 
+struct __shared_ptr_default_allocator<_Yp, true>
+{
+typedef allocator<__shared_ptr_dummy_rebind_allocator_type> type;
+};
 
 template 
 _LIBCPP_INLINE_VISIBILITY
@@ -3939,8 +3964,7 @@
 }
 }
 
-_LIBCPP_INLINE_VISIBILITY
-void __enable_weak_this(const volatile void*, const volatile void*) _NOEXCEPT {}
+_LIBCPP_INLINE_VISIBILITY void __enable_weak_this(...) _NOEXCEPT {}
 
 template  friend class _LIBCPP_TEMPLATE_VIS shared_ptr;
 template  friend class _LIBCPP_TEMPLATE_VIS weak_ptr;
@@ -3972,8 +3996,9 @@
 : __ptr_(__p)
 {
 unique_ptr<_Yp> __hold(__p);
-typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT());
 __hold.release();
 __enable_weak_this(__p, __p);
 }
@@ -3988,8 +4013,9 @@
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, __d, allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, _Dp, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, __d, _AllocT());
 __enable_weak_this(__p, __p);
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
@@ -4010,8 +4036,9 @@
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-typedef __shared_ptr_pointer > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, __d, allocator<_Tp>());
+typedef typename __shared_ptr_default_allocator<_Tp>::type _AllocT;
+typedef __shared_ptr_pointer _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, __d, _AllocT());
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
 catch (...)
@@ -4179,8 +4206,9 @@
 else
 #endif
 {
-typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, _Dp, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), _AllocT());
 __enable_weak_this(__r.get(), __r.get());
 }
 __r.release();
@@ -4208,10 +4236,11 @@
 else
 #endif
 {
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
 typedef __shared_ptr_pointer<_Yp*,
  reference_wrapper::type>,
- allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__r.get(), ref(__r.get_deleter()), allocator<_Yp>());
+ _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__r.get(), ref(__r.get_deleter()), _AllocT());
 __enable_weak_this(__r.get(), __r.get());
 }
 __r.release();
Index: 

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Eric Fiselier via Phabricator via cfe-commits]

EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.

LGTM after addressing inline comments. Sorry for the delay.




Comment at: include/memory:3663
+template 
+struct rebind
+{

This rebind isn't getting selected because it's private, but 
`std::allocator_traits` is doing the rebinding instead.

Please make the rebinding public.


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Ping! @mclow.lists: Do you have any thoughts here?


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 97313.
erik.pilkington added a comment.

Rebase n' ping!


https://reviews.llvm.org/D30837

Files:
  include/memory
  
test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
  
test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp

Index: test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
===
--- test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
+++ test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
@@ -45,6 +45,13 @@
 virtual ~Foo() = default;
 };
 
+struct Result {};
+static Result theFunction() { return Result(); }
+static int resultDeletorCount;
+static void resultDeletor(Result (*pf)()) {
+  assert(pf == theFunction);
+  ++resultDeletorCount;
+}
 
 int main()
 {
@@ -65,7 +72,11 @@
 std::shared_ptr p2 = std::make_shared();
 assert(p2.get());
 }
-
+{ // https://bugs.llvm.org/show_bug.cgi?id=27566
+  std::shared_ptr x(, );
+  std::shared_ptr y(theFunction, resultDeletor);
+}
+assert(resultDeletorCount == 2);
 #if TEST_STD_VER >= 11
 nc = globalMemCounter.outstanding_new;
 {
Index: test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
===
--- test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
+++ test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
@@ -0,0 +1,40 @@
+#include 
+
+template  struct Tag {};
+
+template 
+using SPtr = std::shared_ptr;
+
+template 
+using FnType = void(Tag);
+
+template 
+void TestFn(Tag) {}
+
+template 
+FnType* getFn() {
+  return ;
+}
+
+struct Deleter {
+  template 
+  void operator()(Tp) const {
+using RawT = typename std::remove_pointer::type;
+static_assert(std::is_function::value ||
+  std::is_null_pointer::value, "");
+  }
+};
+
+int main() {
+  {
+SPtr<0> s; // OK
+SPtr<1> s1(nullptr); // OK
+SPtr<2> s2(getFn<2>(), Deleter{}); // OK
+SPtr<3> s3(nullptr, Deleter{}); // OK
+  }
+  // expected-error@memory:* 2 {{static_assert failed "default_delete cannot be instantiated for function types"}}
+  {
+SPtr<4> s4(getFn<4>()); // expected-note {{requested here}}
+SPtr<5> s5(getFn<5>(), std::default_delete>{}); // expected-note {{requested here}}
+  }
+}
Index: include/memory
===
--- include/memory
+++ include/memory
@@ -2251,6 +2251,8 @@
 
 template 
 struct _LIBCPP_TEMPLATE_VIS default_delete {
+static_assert(!is_function<_Tp>::value,
+  "default_delete cannot be instantiated for function types");
 #ifndef _LIBCPP_CXX03_LANG
   _LIBCPP_INLINE_VISIBILITY constexpr default_delete() noexcept = default;
 #else
@@ -3653,6 +3655,17 @@
 __a.deallocate(_PTraits::pointer_to(*this), 1);
 }
 
+struct __shared_ptr_dummy_rebind_allocator_type;
+template <>
+class _LIBCPP_TEMPLATE_VIS allocator<__shared_ptr_dummy_rebind_allocator_type>
+{
+template 
+struct rebind
+{
+typedef allocator<_Other> other;
+};
+};
+
 template class _LIBCPP_TEMPLATE_VIS enable_shared_from_this;
 
 template
@@ -3921,6 +3934,17 @@
 #endif  // _LIBCPP_HAS_NO_VARIADICS
 
 private:
+template ::value>
+struct __shared_ptr_default_allocator
+{
+typedef allocator<_Yp> type;
+};
+
+template 
+struct __shared_ptr_default_allocator<_Yp, true>
+{
+typedef allocator<__shared_ptr_dummy_rebind_allocator_type> type;
+};
 
 template 
 _LIBCPP_INLINE_VISIBILITY
@@ -3936,8 +3960,7 @@
 }
 }
 
-_LIBCPP_INLINE_VISIBILITY
-void __enable_weak_this(const volatile void*, const volatile void*) _NOEXCEPT {}
+_LIBCPP_INLINE_VISIBILITY void __enable_weak_this(...) _NOEXCEPT {}
 
 template  friend class _LIBCPP_TEMPLATE_VIS shared_ptr;
 template  friend class _LIBCPP_TEMPLATE_VIS weak_ptr;
@@ -3968,8 +3991,9 @@
 : __ptr_(__p)
 {
 unique_ptr<_Yp> __hold(__p);
-typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT());
 __hold.release();
 __enable_weak_this(__p, __p);
 }
@@ -3984,8 +4008,9 @@
 try
 {
 #endif  // 

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.



Comment at: include/memory:3606
+template <>
+class _LIBCPP_TEMPLATE_VIS allocator<__shared_ptr_dummy_rebind_allocator_type>
+{

EricWF wrote:
> I would prefer using an entirely different allocator type, not a 
> specialization of `std::allocator`. 
> 
> Re-naming this class to `__shared_ptr_dummy_rebind_allocator` and moving it 
> closer to `__shared_ptr_default_allocator` would be  good.
Unfortunately, for this approach to work we have to specialize `std::allocator` 
because we use the `template  allocator(const allocator<_Up> &)` 
constructor at `__shared_ptr_pointer::__on_zero_shared_weak`. 

If specializing `std::allocator` is a problem, I suppose we could pass in a 
special entirely empty struct as an "allocator" to `__shared_ptr_pointer` when 
necessary, then metaprogram our way out of using the converting constructor at 
`__shared_ptr_ptr::__on_zero_shared_weak`. I think the current approach is 
simpler though.


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 95210.
erik.pilkington added a comment.

This new patch includes @EricWF's static_assert & test.
Thanks,
Erik


https://reviews.llvm.org/D30837

Files:
  include/memory
  
test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
  
test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp

Index: test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
===
--- test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
+++ test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
@@ -45,6 +45,13 @@
 virtual ~Foo() = default;
 };
 
+struct Result {};
+static Result theFunction() { return Result(); }
+static int resultDeletorCount;
+static void resultDeletor(Result (*pf)()) {
+  assert(pf == theFunction);
+  ++resultDeletorCount;
+}
 
 int main()
 {
@@ -65,7 +72,11 @@
 std::shared_ptr p2 = std::make_shared();
 assert(p2.get());
 }
-
+{ // https://bugs.llvm.org/show_bug.cgi?id=27566
+  std::shared_ptr x(, );
+  std::shared_ptr y(theFunction, resultDeletor);
+}
+assert(resultDeletorCount == 2);
 #if TEST_STD_VER >= 11
 nc = globalMemCounter.outstanding_new;
 {
Index: test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
===
--- test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
+++ test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
@@ -0,0 +1,40 @@
+#include 
+
+template  struct Tag {};
+
+template 
+using SPtr = std::shared_ptr;
+
+template 
+using FnType = void(Tag);
+
+template 
+void TestFn(Tag) {}
+
+template 
+FnType* getFn() {
+  return ;
+}
+
+struct Deleter {
+  template 
+  void operator()(Tp) const {
+using RawT = typename std::remove_pointer::type;
+static_assert(std::is_function::value ||
+  std::is_null_pointer::value, "");
+  }
+};
+
+int main() {
+  {
+SPtr<0> s; // OK
+SPtr<1> s1(nullptr); // OK
+SPtr<2> s2(getFn<2>(), Deleter{}); // OK
+SPtr<3> s3(nullptr, Deleter{}); // OK
+  }
+  // expected-error@memory:* 2 {{static_assert failed "default_delete cannot be instantiated for function types"}}
+  {
+SPtr<4> s4(getFn<4>()); // expected-note {{requested here}}
+SPtr<5> s5(getFn<5>(), std::default_delete>{}); // expected-note {{requested here}}
+  }
+}
Index: include/memory
===
--- include/memory
+++ include/memory
@@ -2274,6 +2274,8 @@
 template 
 struct _LIBCPP_TEMPLATE_VIS default_delete
 {
+static_assert(!is_function<_Tp>::value,
+  "default_delete cannot be instantiated for function types");
 #ifndef _LIBCPP_HAS_NO_DEFAULTED_FUNCTIONS
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR default_delete() _NOEXCEPT = default;
 #else
@@ -3493,6 +3495,17 @@
 __a.deallocate(_PTraits::pointer_to(*this), 1);
 }
 
+struct __shared_ptr_dummy_rebind_allocator_type;
+template <>
+class _LIBCPP_TEMPLATE_VIS allocator<__shared_ptr_dummy_rebind_allocator_type>
+{
+template 
+struct rebind
+{
+typedef allocator<_Other> other;
+};
+};
+
 template class _LIBCPP_TEMPLATE_VIS enable_shared_from_this;
 
 template
@@ -3761,6 +3774,17 @@
 #endif  // _LIBCPP_HAS_NO_VARIADICS
 
 private:
+template ::value>
+struct __shared_ptr_default_allocator
+{
+typedef allocator<_Yp> type;
+};
+
+template 
+struct __shared_ptr_default_allocator<_Yp, true>
+{
+typedef allocator<__shared_ptr_dummy_rebind_allocator_type> type;
+};
 
 template 
 _LIBCPP_INLINE_VISIBILITY
@@ -3776,8 +3800,7 @@
 }
 }
 
-_LIBCPP_INLINE_VISIBILITY
-void __enable_weak_this(const volatile void*, const volatile void*) _NOEXCEPT {}
+_LIBCPP_INLINE_VISIBILITY void __enable_weak_this(...) _NOEXCEPT {}
 
 template  friend class _LIBCPP_TEMPLATE_VIS shared_ptr;
 template  friend class _LIBCPP_TEMPLATE_VIS weak_ptr;
@@ -3808,8 +3831,9 @@
 : __ptr_(__p)
 {
 unique_ptr<_Yp> __hold(__p);
-typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT());
 __hold.release();
 

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

- Please apply this patch 
 on top of 
yours. It adds tests for the diagnostics produced when the default deleter is 
used.

LGTM other than the inline comments. I'll want to run this by @mclow.lists 
before giving it gets committed though.




Comment at: include/memory:3606
+template <>
+class _LIBCPP_TEMPLATE_VIS allocator<__shared_ptr_dummy_rebind_allocator_type>
+{

I would prefer using an entirely different allocator type, not a specialization 
of `std::allocator`. 

Re-naming this class to `__shared_ptr_dummy_rebind_allocator` and moving it 
closer to `__shared_ptr_default_allocator` would be  good.


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Ping!


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 93488.
erik.pilkington marked 2 inline comments as done.
erik.pilkington added a comment.

In this new patch, use an explicit specialization of std::allocator that 
specifically only performs a rebind. This needs to be a specialization of 
std::allocator so we can use allocator's `allocator(const allocator &)` ctor 
from `shared_ptr_pointer::__on_zero_shared_weak()`.
Thanks,
Erik


https://reviews.llvm.org/D30837

Files:
  include/memory
  
test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp

Index: test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
===
--- test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
+++ test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
@@ -45,6 +45,13 @@
 virtual ~Foo() = default;
 };
 
+struct Result {};
+static Result theFunction() { return Result(); }
+static int resultDeletorCount;
+static void resultDeletor(Result (*pf)()) {
+  assert(pf == theFunction);
+  ++resultDeletorCount;
+}
 
 int main()
 {
@@ -65,7 +72,11 @@
 std::shared_ptr p2 = std::make_shared();
 assert(p2.get());
 }
-
+{ // https://bugs.llvm.org/show_bug.cgi?id=27566
+  std::shared_ptr x(, );
+  std::shared_ptr y(theFunction, resultDeletor);
+}
+assert(resultDeletorCount == 2);
 #if TEST_STD_VER >= 11
 nc = globalMemCounter.outstanding_new;
 {
Index: include/memory
===
--- include/memory
+++ include/memory
@@ -3601,6 +3601,17 @@
 __a.deallocate(_PTraits::pointer_to(*this), 1);
 }
 
+struct __shared_ptr_dummy_rebind_allocator_type;
+template <>
+class _LIBCPP_TEMPLATE_VIS allocator<__shared_ptr_dummy_rebind_allocator_type>
+{
+template 
+struct rebind
+{
+typedef allocator<_Other> other;
+};
+};
+
 template class _LIBCPP_TEMPLATE_VIS enable_shared_from_this;
 
 template
@@ -3869,6 +3880,17 @@
 #endif  // _LIBCPP_HAS_NO_VARIADICS
 
 private:
+template ::value>
+struct __shared_ptr_default_allocator
+{
+typedef allocator<_Yp> type;
+};
+
+template 
+struct __shared_ptr_default_allocator<_Yp, true>
+{
+typedef allocator<__shared_ptr_dummy_rebind_allocator_type> type;
+};
 
 template 
 _LIBCPP_INLINE_VISIBILITY
@@ -3884,8 +3906,7 @@
 }
 }
 
-_LIBCPP_INLINE_VISIBILITY
-void __enable_weak_this(const volatile void*, const volatile void*) _NOEXCEPT {}
+_LIBCPP_INLINE_VISIBILITY void __enable_weak_this(...) _NOEXCEPT {}
 
 template  friend class _LIBCPP_TEMPLATE_VIS shared_ptr;
 template  friend class _LIBCPP_TEMPLATE_VIS weak_ptr;
@@ -3916,8 +3937,9 @@
 : __ptr_(__p)
 {
 unique_ptr<_Yp> __hold(__p);
-typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT());
 __hold.release();
 __enable_weak_this(__p, __p);
 }
@@ -3932,8 +3954,9 @@
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, __d, allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, _Dp, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, __d, _AllocT());
 __enable_weak_this(__p, __p);
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
@@ -3954,8 +3977,9 @@
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-typedef __shared_ptr_pointer > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, __d, allocator<_Tp>());
+typedef typename __shared_ptr_default_allocator<_Tp>::type _AllocT;
+typedef __shared_ptr_pointer _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, __d, _AllocT());
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
 catch (...)
@@ -4123,8 +4147,9 @@
 else
 #endif
 {
-typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, _Dp, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), _AllocT());
 __enable_weak_this(__r.get(), __r.get());
 }
 

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: include/memory:3933
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > 
_CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT());
 __hold.release();

Quuxplusone wrote:
> EricWF wrote:
> 
> > This patch seems to support constructing a shared_ptr without 
> > providing a non-default deleter. I don't think this should work because the 
> > default deleter will attempt to free a function pointer, which is never 
> > valid. (Although I think this case will still cause a compile error).
> 
> Good point. But then are you suggesting that this constructor should be 
> SFINAEd in that case, or just static_assert, or leave the existing behavior 
> (which is indeed a hard compile error *inside* a static-assert)?
> 
> ```
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2514:27:
>  error: invalid application of
>   'sizeof' to a function type
> static_assert(sizeof(_Tp) > 0, "default_delete can not delete 
> incomplete type");
>   ^~~
> x.cc:6:2: note: in instantiation of member function 
> 'std::__1::default_delete::operator()' requested here
> ```
> 
> (Technically-technically, arguably the user is allowed to fully specialize 
> `std::default_delete` for some user-defined type `T`, right? Not that 
> libc++ ought to be catering to such people.)
SFINAE is definitely the wrong approach, since we don't want to affect overload 
resolution.

On second thought it's probably better to fix the allocator type here anyway, 
since the instantiation of `default_delete` will lead to a more readable 
compile error.

Also you're right about user defined specializations of default_delete, you're 
also right that libc++ ought not care about such craziness. 


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: include/memory:3933
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > 
_CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT());
 __hold.release();

EricWF wrote:

> This patch seems to support constructing a shared_ptr without 
> providing a non-default deleter. I don't think this should work because the 
> default deleter will attempt to free a function pointer, which is never 
> valid. (Although I think this case will still cause a compile error).

Good point. But then are you suggesting that this constructor should be SFINAEd 
in that case, or just static_assert, or leave the existing behavior (which is 
indeed a hard compile error *inside* a static-assert)?

```
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2514:27:
 error: invalid application of
  'sizeof' to a function type
static_assert(sizeof(_Tp) > 0, "default_delete can not delete 
incomplete type");
  ^~~
x.cc:6:2: note: in instantiation of member function 
'std::__1::default_delete::operator()' requested here
```

(Technically-technically, arguably the user is allowed to fully specialize 
`std::default_delete` for some user-defined type `T`, right? Not that 
libc++ ought to be catering to such people.)


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

This patch seems to support constructing a `shared_ptr` without 
providing a non-default deleter. I don't think this should work because the 
default deleter will attempt to free a function pointer, which is never valid. 
(Although I think this case will still cause a compile error).




Comment at: include/memory:3896
+
+template  struct __shared_ptr_default_allocator<_Tp, false>
+{

I would just handle this case with the primary template rather than a complete 
specialization.



Comment at: include/memory:3903
+{
+typedef allocator type;
+};

Using an arbitrary and unrelated allocator type `std::allocator` still 
makes me nervous. I would rather use a custom allocator type written only for 
this use case.


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Ping!


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In https://reviews.llvm.org/D30837#698305, @EricWF wrote:

> We can't just use an arbitrary allocator type for a number of reasons:
>
> - You just changed the type of the control block. That's ABI breaking.


Ah, I didn't think of that. This new patch only selects `allocator` when 
_Yp is a function type, which is only permitted as of this patch.

> - `allocator` allocates ints, nothing else.

Since we only use `allocator` for a rebind, we don't violate this rule.

> - It may mean we don't select a valid user specialization of `allocator`.

In the new patch, `allocator` is only selected if `is_function<_Yp>`, I 
don't believe that there is any valid user specialization of 
`std::allocator`. This is because [namespace.std] p1:

> A program may add a template specialization for any standard library template 
> to namespace std only if [...] the specialization meets the standard library 
> requirements for the original template [...]

In this case, the original template is the default allocator, which is supposed 
to have overloads of `address` taking a `const_reference` and a `reference` 
([allocator.members] p2-3), which will be ambiguous for function types with the 
given definitions of `const_reference` and `reference`. Is this a valid 
argument?


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 91660.
erik.pilkington added a comment.

In this new patch:

- We only select `allocator` when _Yp is a function type, fixing the ABI 
break @EricWF pointed out.
- Only try to select a different allocator if we're using the 
`__shared_ptr_pointer` layout; `make_shared` doesn't make sense.
- Improve the testcase

Thanks for the feedback!
Erik


https://reviews.llvm.org/D30837

Files:
  include/memory
  
test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp

Index: test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
===
--- test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
+++ test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
@@ -45,6 +45,13 @@
 virtual ~Foo() = default;
 };
 
+struct Result {};
+static Result theFunction() { return Result(); }
+static int resultDeletorCount;
+static void resultDeletor(Result (*pf)()) {
+  assert(pf == theFunction);
+  ++resultDeletorCount;
+}
 
 int main()
 {
@@ -65,7 +72,11 @@
 std::shared_ptr p2 = std::make_shared();
 assert(p2.get());
 }
-
+{ // https://bugs.llvm.org/show_bug.cgi?id=27566
+  std::shared_ptr x(, );
+  std::shared_ptr y(theFunction, resultDeletor);
+}
+assert(resultDeletorCount == 2);
 #if TEST_STD_VER >= 11
 nc = globalMemCounter.outstanding_new;
 {
Index: include/memory
===
--- include/memory
+++ include/memory
@@ -3884,13 +3884,25 @@
 }
 }
 
-_LIBCPP_INLINE_VISIBILITY
-void __enable_weak_this(const volatile void*, const volatile void*) _NOEXCEPT {}
+_LIBCPP_INLINE_VISIBILITY void __enable_weak_this(...) _NOEXCEPT {}
 
 template  friend class _LIBCPP_TEMPLATE_VIS shared_ptr;
 template  friend class _LIBCPP_TEMPLATE_VIS weak_ptr;
 };
 
+template ::value>
+struct __shared_ptr_default_allocator;
+
+template  struct __shared_ptr_default_allocator<_Tp, false>
+{
+typedef allocator<_Tp> type;
+};
+
+template  struct __shared_ptr_default_allocator<_Tp, true>
+{
+typedef allocator type;
+};
+
 template
 inline
 _LIBCPP_CONSTEXPR
@@ -3916,8 +3928,9 @@
 : __ptr_(__p)
 {
 unique_ptr<_Yp> __hold(__p);
-typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT());
 __hold.release();
 __enable_weak_this(__p, __p);
 }
@@ -3932,8 +3945,9 @@
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, __d, allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, _Dp, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, __d, _AllocT());
 __enable_weak_this(__p, __p);
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
@@ -3954,8 +3968,9 @@
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-typedef __shared_ptr_pointer > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, __d, allocator<_Tp>());
+typedef typename __shared_ptr_default_allocator<_Tp>::type _AllocT;
+typedef __shared_ptr_pointer _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, __d, _AllocT());
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
 catch (...)
@@ -4094,8 +4109,9 @@
 typename enable_if::value, __nat>::type)
 : __ptr_(__r.get())
 {
-typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__r.get(), default_delete<_Yp>(), allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__r.get(), default_delete<_Yp>(), _AllocT());
 __enable_weak_this(__r.get(), __r.get());
 __r.release();
 }
@@ -4123,8 +4139,9 @@
 else
 #endif
 {
-typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), allocator<_Yp>());
+typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+typedef __shared_ptr_pointer<_Yp*, _Dp, _AllocT > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), _AllocT());

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Eric Fiselier via Phabricator via cfe-commits]

EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.

We can't just use an arbitrary allocator type for a number of reasons:

- You just changed the type of the control block. That's ABI breaking.
- `allocator` allocates ints, nothing else.
- It may mean we don't select a valid user specialization of `allocator`.

I'll play around to see if I can come up with another path forward to fix this.


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

I notice that the Standard implies that std::unique_ptr will work only with 
*object types* T, and yet the "object" wording is missing from shared_ptr.

> A unique pointer is an object that owns another *object* and manages that 
> other *object* through a pointer.



> The shared_ptr class template stores a *pointer*, usually obtained via new. 
> shared_ptr implements semantics of shared ownership; the last remaining owner 
> of the pointer is responsible for destroying the object, *or otherwise 
> releasing the resources associated with the stored pointer.*

So it does seem like shared_ptr should work and should store a T(*)().
And it does seem that unique_ptr should continue to 
not-work... but maybe there's a useful vendor extension or standards proposal 
to be made in that area.




Comment at: include/memory:4209
 {
-typedef __shared_ptr_emplace<_Tp, allocator<_Tp> > _CntrlBlk;
+typedef __shared_ptr_emplace<_Tp, allocator > _CntrlBlk;
 typedef allocator<_CntrlBlk> _Alloc2;

IIUC, you don't need to touch make_shared. I doubt these changes are truly 
harmful, but I don't understand what meaning we're trying to assign to things 
like

auto a = std::make_shared();
auto b = std::make_shared(myFunction);

It seems like the effect of this change is just to mess with anyone who's 
explicitly specializing std::allocator to make its ::rebind 
member do something wacky. In which case I'd say they've got it coming to 
them... but the effect of this change doesn't seem relevant to the purpose 
described in your commit message. :)



Comment at: 
test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp:49
+struct Result {};
+void resultDeletor(Result (*)()) {}
+Result theFunction() { return Result(); }

Could you plausibly make this

void resultDeleter(Result (*f)()) { assert(f == theFunction); }

for extra sanity checking?
For super-duper checking, touch a global variable in `resultDeleter` so that 
you can verify that the deleter was actually called. This might reasonably be 
perceived as overkill. :)



Comment at: 
test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp:72
+{ // https://bugs.llvm.org/show_bug.cgi?id=27566
+  std::shared_ptr x(, );
+}

Presumably this should also compile without the explicit `&`s, right? Might be 
worth adding

std::shared_ptr y(theFunction, resultDeleter);

in addition to your existing test case, just to make sure that omitting the `&` 
doesn't mess up the template deduction somehow.
IIUC, it would never make sense to write something like

auto z = std::make_shared(theFunction);

so that part doesn't need testing, right?


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 91385.
erik.pilkington added a comment.

This new patch replaces the allocator from `allocator` to 
`allocator`, I didn't realize `allocator` was deprecated.
Thanks,
Erik


https://reviews.llvm.org/D30837

Files:
  include/memory
  
test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp

Index: test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
===
--- test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
+++ test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
@@ -45,6 +45,9 @@
 virtual ~Foo() = default;
 };
 
+struct Result {};
+void resultDeletor(Result (*)()) {}
+Result theFunction() { return Result(); }
 
 int main()
 {
@@ -65,7 +68,9 @@
 std::shared_ptr p2 = std::make_shared();
 assert(p2.get());
 }
-
+{ // https://bugs.llvm.org/show_bug.cgi?id=27566
+  std::shared_ptr x(, );
+}
 #if TEST_STD_VER >= 11
 nc = globalMemCounter.outstanding_new;
 {
Index: include/memory
===
--- include/memory
+++ include/memory
@@ -3884,8 +3884,7 @@
 }
 }
 
-_LIBCPP_INLINE_VISIBILITY
-void __enable_weak_this(const volatile void*, const volatile void*) _NOEXCEPT {}
+_LIBCPP_INLINE_VISIBILITY void __enable_weak_this(...) _NOEXCEPT {}
 
 template  friend class _LIBCPP_TEMPLATE_VIS shared_ptr;
 template  friend class _LIBCPP_TEMPLATE_VIS weak_ptr;
@@ -3916,8 +3915,8 @@
 : __ptr_(__p)
 {
 unique_ptr<_Yp> __hold(__p);
-typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), allocator<_Yp>());
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), allocator());
 __hold.release();
 __enable_weak_this(__p, __p);
 }
@@ -3932,8 +3931,8 @@
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, __d, allocator<_Yp>());
+typedef __shared_ptr_pointer<_Yp*, _Dp, allocator > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, __d, allocator());
 __enable_weak_this(__p, __p);
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
@@ -3954,8 +3953,8 @@
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-typedef __shared_ptr_pointer > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, __d, allocator<_Tp>());
+typedef __shared_ptr_pointer _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, __d, allocator());
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
 catch (...)
@@ -4094,8 +4093,8 @@
 typename enable_if::value, __nat>::type)
 : __ptr_(__r.get())
 {
-typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__r.get(), default_delete<_Yp>(), allocator<_Yp>());
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__r.get(), default_delete<_Yp>(), allocator());
 __enable_weak_this(__r.get(), __r.get());
 __r.release();
 }
@@ -4123,8 +4122,8 @@
 else
 #endif
 {
-typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), allocator<_Yp>());
+typedef __shared_ptr_pointer<_Yp*, _Dp, allocator > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), allocator());
 __enable_weak_this(__r.get(), __r.get());
 }
 __r.release();
@@ -4154,8 +4153,8 @@
 {
 typedef __shared_ptr_pointer<_Yp*,
  reference_wrapper::type>,
- allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__r.get(), ref(__r.get_deleter()), allocator<_Yp>());
+ allocator > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__r.get(), ref(__r.get_deleter()), allocator());
 __enable_weak_this(__r.get(), __r.get());
 }
 __r.release();
@@ -4168,12 +4167,13 @@
 shared_ptr<_Tp>
 shared_ptr<_Tp>::make_shared(_Args&& ...__args)
 {
-typedef __shared_ptr_emplace<_Tp, allocator<_Tp> > _CntrlBlk;
+typedef __shared_ptr_emplace<_Tp, allocator > _CntrlBlk;
 typedef allocator<_CntrlBlk> _A2;
 typedef __allocator_destructor<_A2> _D2;
 _A2 __a2;
 unique_ptr<_CntrlBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
-::new(__hold2.get()) _CntrlBlk(__a2, 

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Andrey Khalyavin via Phabricator via cfe-commits]

halyavin added a comment.

But std::allocator is deprecated in C++17. I don't know a good solution, 
I just used int as an arbitrary type when I faced similar problem.


https://reviews.llvm.org/D30837



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington created this revision.

This patch adds support for `shared_ptr` types, so that the following 
works:

  void Func();
  void Del(void (*)())
  std::shared_ptr x(Func, Del);

Where previously this would fail to compile. In PR27566, the use case described 
for this was a shared pointer to a dynamically loaded function, which seems 
reasonable enough to me. Both libstdc++ and MSVC support this use case as well, 
and I didn't notice any wording in the standard that disallowed it. This patch 
has 2 parts:

1. Don't try to instantiate an `allocator`
2. Fix `__enable_weak_this()` dummy overload to work with function pointers.

Number 1 is accomplished by passing in `allocator` into 
`__shared_ptr_pointer` or `__shared_ptr_emplace` when no allocator was passed 
to the constructor of the `shared_ptr`, I believe this is correct because in 
this case the allocator is only used for a rebind, so any instantiation of 
std::allocator would work fine.

This is my first libc++ patch, so take it with a grain of salt! Thanks for 
taking a look,
Erik


https://reviews.llvm.org/D30837

Files:
  include/memory
  
test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp

Index: test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
===
--- test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
+++ test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp
@@ -45,6 +45,9 @@
 virtual ~Foo() = default;
 };
 
+struct Result {};
+void resultDeletor(Result (*)()) {}
+Result theFunction() { return Result(); }
 
 int main()
 {
@@ -65,7 +68,9 @@
 std::shared_ptr p2 = std::make_shared();
 assert(p2.get());
 }
-
+{ // https://bugs.llvm.org/show_bug.cgi?id=27566
+  std::shared_ptr x(, );
+}
 #if TEST_STD_VER >= 11
 nc = globalMemCounter.outstanding_new;
 {
Index: include/memory
===
--- include/memory
+++ include/memory
@@ -3884,8 +3884,7 @@
 }
 }
 
-_LIBCPP_INLINE_VISIBILITY
-void __enable_weak_this(const volatile void*, const volatile void*) _NOEXCEPT {}
+_LIBCPP_INLINE_VISIBILITY void __enable_weak_this(...) _NOEXCEPT {}
 
 template  friend class _LIBCPP_TEMPLATE_VIS shared_ptr;
 template  friend class _LIBCPP_TEMPLATE_VIS weak_ptr;
@@ -3916,8 +3915,8 @@
 : __ptr_(__p)
 {
 unique_ptr<_Yp> __hold(__p);
-typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), allocator<_Yp>());
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), allocator());
 __hold.release();
 __enable_weak_this(__p, __p);
 }
@@ -3932,8 +3931,8 @@
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, __d, allocator<_Yp>());
+typedef __shared_ptr_pointer<_Yp*, _Dp, allocator > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, __d, allocator());
 __enable_weak_this(__p, __p);
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
@@ -3954,8 +3953,8 @@
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-typedef __shared_ptr_pointer > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__p, __d, allocator<_Tp>());
+typedef __shared_ptr_pointer _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__p, __d, allocator());
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
 catch (...)
@@ -4094,8 +4093,8 @@
 typename enable_if::value, __nat>::type)
 : __ptr_(__r.get())
 {
-typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__r.get(), default_delete<_Yp>(), allocator<_Yp>());
+typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, allocator > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__r.get(), default_delete<_Yp>(), allocator());
 __enable_weak_this(__r.get(), __r.get());
 __r.release();
 }
@@ -4123,8 +4122,8 @@
 else
 #endif
 {
-typedef __shared_ptr_pointer<_Yp*, _Dp, allocator<_Yp> > _CntrlBlk;
-__cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), allocator<_Yp>());
+typedef __shared_ptr_pointer<_Yp*, _Dp, allocator > _CntrlBlk;
+__cntrl_ = new _CntrlBlk(__r.get(), __r.get_deleter(), allocator());
 __enable_weak_this(__r.get(), __r.get());
 }
 __r.release();
@@ -4154,8 +4153,8 @@
 {
 typedef __shared_ptr_pointer<_Yp*,