[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

76

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

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

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/docs/Cxx1zStatusIssuesStatus.csv

 "`2855 <https://wg21.link/LWG2855>`__","std::throw_with_nested(""string_literal"")","Kona","|Complete|",""
 "`2857 <https://wg21.link/LWG2857>`__","{variant,optional,any}::emplace should return the constructed value","Kona","|Complete|",""
 "`2861 <https://wg21.link/LWG2861>`__","basic_string should require that charT match traits::char_type","Kona","|Complete|",""
 "`2866 <https://wg21.link/LWG2866>`__","Incorrect derived classes constraints","Kona","|Nothing To Do|",""
 "`2868 <https://wg21.link/LWG2868>`__","Missing specification of bad_any_cast::what()","Kona","|Complete|",""
 "`2872 <https://wg21.link/LWG2872>`__","Add definition for direct-non-list-initialization","Kona","|Complete|",""
 "`2873 <https://wg21.link/LWG2873>`__","Add noexcept to several shared_ptr related functions","Kona","|Complete|",""
 "`2874 <https://wg21.link/LWG2874>`__","Constructor ``shared_ptr::shared_ptr(Y*)``\  should be constrained","Kona","",""
-"`2875 <https://wg21.link/LWG2875>`__","shared_ptr::shared_ptr(Y\*, D, [|hellip|\ ]) constructors should be constrained","Kona","",""
+"`2875 <https://wg21.link/LWG2875>`__","shared_ptr::shared_ptr(Y\*, D, [|hellip|\ ]) constructors should be constrained","Kona","|Complete|",""
 "`2876 <https://wg21.link/LWG2876>`__","``shared_ptr::shared_ptr(const weak_ptr<Y>&)``\  constructor should be constrained","Kona","",""
 "`2878 <https://wg21.link/LWG2878>`__","Missing DefaultConstructible requirement for istream_iterator default constructor","Kona","|Complete|",""
 "`2890 <https://wg21.link/LWG2890>`__","The definition of 'object state' applies only to class types","Kona","|Complete|",""
 "`2900 <https://wg21.link/LWG2900>`__","The copy and move constructors of optional are not constexpr","Kona","|Complete|",""
 "`2903 <https://wg21.link/LWG2903>`__","The form of initialization for the emplace-constructors is not specified","Kona","|Complete|",""
 "`2904 <https://wg21.link/LWG2904>`__","Make variant move-assignment more exception safe","Kona","|Complete|",""
 "`2905 <https://wg21.link/LWG2905>`__","is_constructible_v<unique_ptr<P, D>, P, D const &> should be false when D is not copy constructible","Kona","|Complete|",""
 "`2908 <https://wg21.link/LWG2908>`__","The less-than operator for shared pointers could do more","Kona","|Complete|",""
 "`2911 <https://wg21.link/LWG2911>`__","An is_aggregate type trait is needed","Kona","|Complete|",""
 "`2921 <https://wg21.link/LWG2921>`__","packaged_task and type-erased allocators","Kona","|Complete|",""
 "`2934 <https://wg21.link/LWG2934>`__","optional<const T> doesn't compare with T","Kona","|Complete|",""
 "","","","",""
 "`2901 <https://wg21.link/LWG2901>`__","Variants cannot properly support allocators","Toronto","|Complete|",""
 "`2955 <https://wg21.link/LWG2955>`__","``to_chars / from_chars``\  depend on ``std::string``\ ","Toronto","Resolved by `P0682R1 <https://wg21.link/P0682R1>`__",""
 "`2956 <https://wg21.link/LWG2956>`__","``filesystem::canonical()``\  still defined in terms of ``absolute(p, base)``\ ","Toronto","|Complete|",""

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;
+};
+
 #if defined(_LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI)
 #  define _LIBCPP_SHARED_PTR_TRIVIAL_ABI __attribute__((trivial_abi))
 #else
 #  define _LIBCPP_SHARED_PTR_TRIVIAL_ABI
 #endif
 
 template<class _Tp>
 class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr
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/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp

static_assert(std::is_function<RawT>::value ||
                   std::is_same<typename std::remove_cv<RawT>::type,
                                std::nullptr_t>::value,
                   "");
   }
 };
 
 int main(int, char**) {
   {
     SPtr<0> s; // OK
     SPtr<1> s1(nullptr); // OK
     SPtr<2> s2(getFn<2>(), Deleter{}); // OK
     SPtr<3> s3(nullptr, Deleter{}); // OK
   }
   // expected-error-re@memory:* 2 {{static_assert failed{{.*}} "default_delete cannot be instantiated for function types"}}
   {
     SPtr<4> s4(getFn<4>()); // expected-note {{requested here}}
+    // expected-error@+1 {{no matching constructor for initialization of 'SPtr<5>'}}
     SPtr<5> s5(getFn<5>(), std::default_delete<FnType<5>>{}); // expected-note {{requested here}}
   }
 
   return 0;
 }

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, "");
+
 struct Base { };
 struct Derived : Base { };
 
 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);
     assert(test_deleter<A>::count == 1);
     assert(test_deleter<A>::dealloc_count == 0);
 #ifndef TEST_HAS_NO_RTTI
     test_deleter<A>* d = std::get_deleter<test_deleter<A> >(p);
     assert(d);
     assert(d->state() == 3);
 #endif
     }
     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, "");
+
         // Make sure that we can construct a shared_ptr where the element type and pointer type
         // aren't "convertible" but are "compatible".
         static_assert(!std::is_constructible<std::shared_ptr<Derived[4]>, Base[4], test_deleter<Derived[4]> >::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, "");
+
 struct Base { };
 struct Derived : Base { };
 
 int main(int, char**)
 {
     {
     A* ptr = new A;
     std::shared_ptr<A> p(ptr, test_deleter<A>(3), test_allocator<A>(5));

 #endif
     }
     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, "");
+
         // Make sure that we can construct a shared_ptr where the element type and pointer type
         // aren't "convertible" but are "compatible".
         static_assert(!std::is_constructible<std::shared_ptr<Derived[4]>,
                                              Base[4], test_deleter<Derived[4]>,
                                              test_allocator<Derived[4]> >::value, "");
     }
 
   return 0;
 }