[libcxx] Fix LWG 2875: shared_ptr::shared_ptr(Y*, D, […]) constructors should be constrained.

[zoecarver]

Summary

Fixes LWG issue 2875.

[ldionne]

Do we have tests that shared_ptr isn't constructible from incompatible array types? If not, could we add some?

Otherwise, mostly LGTM.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp

71

Nit: Please don't wrap here.

[zoecarver]

• Fix line wrapping

[zoecarver]

Do we have tests that shared_ptr isn't constructible from incompatible array types? If not, could we add some?

The __compatible_with trait removes the array extent. And (AFAIK) there are no array types that are convertible where their elements aren't. I'll make another patch to add tests that ensure we *can* construct a shared_ptr with array types that aren't convertible but where their elements are.

[zoecarver]

• Rebase off main.

[ldionne]

LGTM. I assume the array part of the requirements in https://wg21.link/LWG2875 are handled in your arrays-in-shared-ptr patch (which I need to get back to reviewing)?

Feel free to ship it after confirmation that my understanding above is right and after CI finishes. Thanks!

[zoecarver]

LGTM. I assume the array part of the requirements in https://wg21.link/LWG2875 are handled in your arrays-in-shared-ptr patch (which I need to get back to reviewing)?

I don't think the wording changes anything about how arrays are handled. As I understand it the deleter requirements are the same for array types and non-array types. Also, the arrays-in-shared-ptr patch landed: D62259. A shared_ptr with array types still uses the same overload, that's what the __compatible_with trait is in charge of.

[ldionne]

Sorry, I got confused with the patch for enabling arrays in make_shared and allocate_shared. For some reason I thought those two were more related than they actually are. Still LGTM.

[zoecarver]

• Fix test failing for C++2b

[zoecarver]

libcxx/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp

42 ↗

(On Diff #324701)

I think I'm going to remove this test and replace it with a test that just checks we hit a static_assert when instantiating default_delete with a function type; no need to get shared_ptr involved.

[zoecarver]

• Remove shared_ptr from tests for default_delete.

[zoecarver]

libcxx/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp

42 ↗

(On Diff #324701)

Actually, never mind. I just removed shared_ptr from the part that was failing (because it added extra errors because a shared_ptr couldn't be constructed because it SFINAEed out). I think the other parts of this test are actually good to have. Anyway, hopefully that fixes the CI.

libcxx/include/memory

 template<class _Tp, class _Up>
 struct __compatible_with
 #if _LIBCPP_STD_VER > 14
     : is_convertible<remove_extent_t<_Tp>*, remove_extent_t<_Up>*> {};
 #else
     : is_convertible<_Tp*, _Up*> {};
 #endif // _LIBCPP_STD_VER > 14
 
+template <class _Dp, class _Pt,
+    class = decltype(_VSTD::declval<_Dp>()(_VSTD::declval<_Pt>()))>
+static true_type __well_formed_deleter_test(int);
+
+template <class, class>
+static false_type __well_formed_deleter_test(...);
+
+template <class _Dp, class _Pt>
+struct __well_formed_deleter : decltype(__well_formed_deleter_test<_Dp, _Pt>(0)) {};
+
+template<class _Dp, class _Tp, class _Yp>
+struct __shared_ptr_deleter_ctor_reqs
+{
+    static const bool value = __compatible_with<_Tp, _Yp>::value &&
+                              is_move_constructible<_Dp>::value &&
+                              __well_formed_deleter<_Dp, _Tp*>::value;
+};
+
 template<class _Tp>
 class _LIBCPP_TEMPLATE_VIS shared_ptr
 {
 public:
 #if _LIBCPP_STD_VER > 14
     typedef weak_ptr<_Tp> weak_type;
     typedef remove_extent_t<_Tp> element_type;
 #else
public:
     _LIBCPP_CONSTEXPR shared_ptr() _NOEXCEPT;
     _LIBCPP_INLINE_VISIBILITY
     _LIBCPP_CONSTEXPR shared_ptr(nullptr_t) _NOEXCEPT;
     template<class _Yp>
         explicit shared_ptr(_Yp* __p,
                             typename enable_if<__compatible_with<_Yp, element_type>::value, __nat>::type = __nat());
     template<class _Yp, class _Dp>
         shared_ptr(_Yp* __p, _Dp __d,
-                   typename enable_if<__compatible_with<_Yp, element_type>::value, __nat>::type = __nat());
+                   typename enable_if<__shared_ptr_deleter_ctor_reqs<_Dp, _Yp, element_type>::value, __nat>::type = __nat());
     template<class _Yp, class _Dp, class _Alloc>
         shared_ptr(_Yp* __p, _Dp __d, _Alloc __a,
-                   typename enable_if<__compatible_with<_Yp, element_type>::value, __nat>::type = __nat());
+                   typename enable_if<__shared_ptr_deleter_ctor_reqs<_Dp, _Yp, element_type>::value, __nat>::type = __nat());
     template <class _Dp> shared_ptr(nullptr_t __p, _Dp __d);
     template <class _Dp, class _Alloc> shared_ptr(nullptr_t __p, _Dp __d, _Alloc __a);
     template<class _Yp> _LIBCPP_INLINE_VISIBILITY shared_ptr(const shared_ptr<_Yp>& __r, element_type* __p) _NOEXCEPT;
     _LIBCPP_INLINE_VISIBILITY
     shared_ptr(const shared_ptr& __r) _NOEXCEPT;
     template<class _Yp>
         _LIBCPP_INLINE_VISIBILITY
         shared_ptr(const shared_ptr<_Yp>& __r,
shared_ptr<_Tp>::shared_ptr(_Yp* __p,
     __cntrl_ = new _CntrlBlk(__p, __shared_ptr_default_delete<_Tp, _Yp>(), _AllocT());
     __hold.release();
     __enable_weak_this(__p, __p);
 }
 
 template<class _Tp>
 template<class _Yp, class _Dp>
 shared_ptr<_Tp>::shared_ptr(_Yp* __p, _Dp __d,
-                            typename enable_if<__compatible_with<_Yp, element_type>::value, __nat>::type)
+                            typename enable_if<__shared_ptr_deleter_ctor_reqs<_Dp, _Yp, element_type>::value, __nat>::type)
     : __ptr_(__p)
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
     try
     {
 #endif  // _LIBCPP_NO_EXCEPTIONS
         typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
         typedef __shared_ptr_pointer<_Yp*, _Dp, _AllocT > _CntrlBlk;
catch (...)
         throw;
     }
 #endif  // _LIBCPP_NO_EXCEPTIONS
 }
 
 template<class _Tp>
 template<class _Yp, class _Dp, class _Alloc>
 shared_ptr<_Tp>::shared_ptr(_Yp* __p, _Dp __d, _Alloc __a,
-                            typename enable_if<__compatible_with<_Yp, element_type>::value, __nat>::type)
+                            typename enable_if<__shared_ptr_deleter_ctor_reqs<_Dp, _Yp, element_type>::value, __nat>::type)
     : __ptr_(__p)
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
     try
     {
 #endif  // _LIBCPP_NO_EXCEPTIONS
         typedef __shared_ptr_pointer<_Yp*, _Dp, _Alloc> _CntrlBlk;
         typedef typename __allocator_traits_rebind<_Alloc, _CntrlBlk>::type _A2;

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp

struct A
 
     A() {++count;}
     A(const A&) {++count;}
     ~A() {--count;}
 };
 
 int A::count = 0;
 
+struct bad_ty { };
+
+struct bad_deleter
+{
+    void operator()(bad_ty) { }
+};
+
+struct no_move_deleter
+{
+    no_move_deleter(no_move_deleter const&) = delete;
+    no_move_deleter(no_move_deleter &&) = delete;
+    void operator()(int*) { }
+};
+
+static_assert(!std::is_move_constructible<no_move_deleter>::value, "");
+
 int main(int, char**)
 {
     {
     A* ptr = new A;
     std::shared_ptr<A> p(ptr, test_deleter<A>(3));
     assert(A::count == 1);
     assert(p.use_count() == 1);
     assert(p.get() == ptr);
     test_deleter<A>* d = std::get_deleter<test_deleter<A> >(p);
     assert(test_deleter<A>::count == 1);
     assert(test_deleter<A>::dealloc_count == 0);
     assert(d);
     assert(d->state() == 3);
     }
     assert(A::count == 0);
     assert(test_deleter<A>::count == 0);
     assert(test_deleter<A>::dealloc_count == 1);
 
+    {
+        // Make sure we can't construct with:
+        //    a) a deleter that doesn't have an operator ()(int*)
+        //    b) a deleter that doesn't have a move constructor.
+        static_assert(!std::is_constructible<std::shared_ptr<int>, int*, bad_deleter>::value, "");
+        static_assert(!std::is_constructible<std::shared_ptr<int>, int*, no_move_deleter>::value,
+                      "");
+    }
+
   return 0;
 }

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter_allocator.pass.cpp

struct A
 
     A() {++count;}
     A(const A&) {++count;}
     ~A() {--count;}
 };
 
 int A::count = 0;
 
+struct bad_ty { };
+
+struct bad_deleter
+{
+    void operator()(bad_ty) { }
+};
+
+struct no_move_deleter
+{
+    no_move_deleter(no_move_deleter const&) = delete;
+    no_move_deleter(no_move_deleter &&) = delete;
+    void operator()(int*) { }
+};
+
+static_assert(!std::is_move_constructible<no_move_deleter>::value, "");
+
 
 int main(int, char**)
 {
     {
     A* ptr = new A;
     std::shared_ptr<A> p(ptr, test_deleter<A>(3), test_allocator<A>(5));
     assert(A::count == 1);
     assert(p.use_count() == 1);
#if TEST_STD_VER >= 11
     assert(d);
     assert(d->state() == 3);
     }
     assert(A::count == 0);
     assert(test_deleter<A>::count == 0);
     assert(test_deleter<A>::dealloc_count == 1);
 #endif
 
+    {
+        // Make sure we can't construct with:
+        //    a) a deleter that doesn't have an operator ()(int*)
+        //    b) a deleter that doesn't have a move constructor.
+        static_assert(!std::is_constructible<std::shared_ptr<int>, int*, bad_deleter,
+                                             test_allocator<A> >::value, "");
+        static_assert(!std::is_constructible<std::shared_ptr<int>, int*, no_move_deleter,
+                                             test_allocator<A> >::value, "");
+    }
+
   return 0;
 }

libcxx/www/cxx1z_status.html

<!--   <I>Note: "NAD" means that the issue was deemed "Not a defect"</I> -->
   <tr><td><a href="https://wg21.link/LWG2855">2855</a></td><td>std::throw_with_nested("string_literal")</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2857">2857</a></td><td>{variant,optional,any}::emplace should return the constructed value</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2861">2861</a></td><td>basic_string should require that charT match traits::char_type</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2866">2866</a></td><td>Incorrect derived classes constraints</td><td>Kona</td><td><I>Nothing to do</I></td></tr>
   <tr><td><a href="https://wg21.link/LWG2868">2868</a></td><td>Missing specification of bad_any_cast::what()</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2872">2872</a></td><td>Add definition for direct-non-list-initialization</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2873">2873</a></td><td>Add noexcept to several shared_ptr related functions</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2874">2874</a></td><td>Constructor shared_ptr::shared_ptr(Y*) should be constrained</td><td>Kona</td><td></td></tr>
-  <tr><td><a href="https://wg21.link/LWG2875">2875</a></td><td>shared_ptr::shared_ptr(Y*, D, [&hellip;]) constructors should be constrained</td><td>Kona</td><td></td></tr>
+  <tr><td><a href="https://wg21.link/LWG2875">2875</a></td><td>shared_ptr::shared_ptr(Y*, D, [&hellip;]) constructors should be constrained</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2876">2876</a></td><td>shared_ptr::shared_ptr(const weak_ptr&lt;Y&gt;&amp;) constructor should be constrained</td><td>Kona</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2878">2878</a></td><td>Missing DefaultConstructible requirement for istream_iterator default constructor</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2890">2890</a></td><td>The definition of 'object state' applies only to class types</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2900">2900</a></td><td>The copy and move constructors of optional are not constexpr</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2903">2903</a></td><td>The form of initialization for the emplace-constructors is not specified</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2904">2904</a></td><td>Make variant move-assignment more exception safe</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2905">2905</a></td><td>is_constructible_v&lt;unique_ptr&lt;P, D&gt;, P, D const &amp;&gt; should be false when D is not copy constructible</td><td>Kona</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2908">2908</a></td><td>The less-than operator for shared pointers could do more</td><td>Kona</td><td>Complete</td></tr>