[libcxx] Fix LWG 2874: Constructor shared_ptr::shared_ptr(Y*) should be constrained.

[ldionne]

Summary

Fix LWG 2874.

[ldionne]

Nitpick: You've got the wrong LWG number in your commit message.

Also, can you please mark the LWG issue as complete in the docs?

libcxx/include/memory

3565 ↗

(On Diff #269307)

You are eagerly instantiating all templates here, and I don't think that's what you want. For example, you'll instantiate __well_formed_delete_op<_Yp*> even when is_array<_Tp>::value is true.

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

122

STD_TEST_VERSION doesn't exist, it's TEST_STD_VER.

To catch these sorts of errors, it can be good to add tests and witness their failure before actually fixing them.

[zoecarver]

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

122

Oops, you're right. Somehow that got suck in my clipboard history.

[zoecarver]

• Update status.
• Use TEST_STD_VER instead of the non-existant STD_TEST_VERSION.
• Use std::conditional (instead of logical and/or).

[zoecarver]

• Rebase off main.

[zoecarver]

• Only use sfinae after C++03.

[ldionne]

Rebase onto <memory> changes and refactor the patch for clarity.

[zoecarver]

Two nits, feel free to take or leave them. LGTM.

libcxx/include/__memory/shared_ptr.h

382

Nit: spaces around =.

439

Another nit, feel free to take it or leave it: I don't think we need to introduce _And and _If here. Why don't we just do:

_EnableIf<
  __compatible_with<_Yp, _Tp>::value 
#if
  && (is_array<_Tp>::value ? 
        __is_array_deletable<_Yp*>::value : 
        __is_deletable<_Yp*>::value)
#endif
>

[ldionne]

Fix spacing.

libcxx/include/__memory/shared_ptr.h

439

The problem here is that we only want to instantiate __is_array_deletable<_Yp*> or __is_deletable<_Yp*> when is_array<_Tp> is true/false, respectively. With your approach, we do it eagerly.

Similarly, we could do this:

__compatible_with<_Yp, _Tp>::value
#ifndef _LIBCPP_CXX03_LANG
  && _If<is_array<_Tp>::value, __is_array_deletable<_Yp*>, __is_deletable<_Yp*> >::value
#endif

But then, if the types are not compatible, we'll still be instantiating one check for __is_array_deletable or __is_deletable, depending on whether the type is an array.

The current approach delays and short cirtcuits the instantiation of SFINAE helpers as long as is possible. The only thing we do check eagerly is is_array<_Tp> regardless of the result of __compatible_with.

Furthermore, it used to be the case that we'd want to minimize these sorts of control structures in TMP code because they were heavier. However, nowadays we're using aliases to implement them, and some folks like Odin Holmes have demonstrated that they are very cheap.

[ldionne]

Will ship once CI is finished (assuming it's green).

[zoecarver]

libcxx/include/__memory/shared_ptr.h

439

Fair enough. I'm convinced :)

And I suppose you're right, the SFINAE type traits are going to be more expensive than _And and _If, so it makes sense to eagerly instantiate only the latter, even if it means slightly more code.

[ldionne]

Try to fix the GCC build.

libcxx/docs/Cxx1zStatusIssuesStatus.csv

 "`2853 <https://wg21.link/LWG2853>`__","Possible inconsistency in specification of erase in [vector.modifiers]","Kona","|Complete|",""
 "`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","",""
+"`2874 <https://wg21.link/LWG2874>`__","Constructor ``shared_ptr::shared_ptr(Y*)``\  should be constrained","Kona","|Complete|","13.0"
 "`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/shared_ptr.h

 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 _Ptr, class=void>
+struct __is_deletable : false_type { };
+template <class _Ptr>
+struct __is_deletable<_Ptr, decltype(delete _VSTD::declval<_Ptr>())> : true_type { };
+
+template <class _Ptr, class=void>
+struct __is_array_deletable : false_type { };
+template <class _Ptr>
+struct __is_array_deletable<_Ptr, decltype(delete[] _VSTD::declval<_Ptr>())> : true_type { };
+
 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>
private:
     __shared_weak_count* __cntrl_;
 
     struct __nat {int __for_bool_;};
 public:
     _LIBCPP_INLINE_VISIBILITY
     _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 = _EnableIf<
+        _And<
+            __compatible_with<_Yp, _Tp>
+            // In C++03 we get errors when trying to do SFINAE with the
+            // delete operator, so we always pretend that it's deletable.
+#ifndef _LIBCPP_CXX03_LANG
+            , _If<is_array<_Tp>::value, __is_array_deletable<_Yp*>, __is_deletable<_Yp*> >
+#endif
+        >::value
+    > >
+    explicit shared_ptr(_Yp* __p) : __ptr_(__p) {
+        unique_ptr<_Yp> __hold(__p);
+        typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
+        typedef __shared_ptr_pointer<_Yp*, __shared_ptr_default_delete<_Tp, _Yp>, _AllocT > _CntrlBlk;
+        __cntrl_ = new _CntrlBlk(__p, __shared_ptr_default_delete<_Tp, _Yp>(), _AllocT());
+        __hold.release();
+        __enable_weak_this(__p, __p);
+    }
+
     template<class _Yp, class _Dp>
         shared_ptr(_Yp* __p, _Dp __d,
                    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<__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);

 _LIBCPP_CONSTEXPR
 shared_ptr<_Tp>::shared_ptr(nullptr_t) _NOEXCEPT
     : __ptr_(nullptr),
       __cntrl_(nullptr)
 {
 }
 
 template<class _Tp>
-template<class _Yp>
-shared_ptr<_Tp>::shared_ptr(_Yp* __p,
-                            typename enable_if<__compatible_with<_Yp, element_type>::value, __nat>::type)
-    : __ptr_(__p)
-{
-    unique_ptr<_Yp> __hold(__p);
-    typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
-    typedef __shared_ptr_pointer<_Yp*, __shared_ptr_default_delete<_Tp, _Yp>, _AllocT > _CntrlBlk;
-    __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<__shared_ptr_deleter_ctor_reqs<_Dp, _Yp, element_type>::value, __nat>::type)
     : __ptr_(__p)
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
     try
     {

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

struct C
 
     C() {++count;}
     C(const C&) {++count;}
     virtual ~C() {--count;}
 };
 
 int C::count = 0;
 
+class private_delete_op
+{
+    static void operator delete (void *p) {
+        return delete static_cast<char*>(p);
+    }
+public:
+    static void operator delete[] (void *p) {
+        return delete[] static_cast<char*>(p);
+    }
+};
+
+class private_delete_arr_op
+{
+    static void operator delete[] (void *p) {
+        return delete[] static_cast<char*>(p);
+    }
+public:
+    static void operator delete (void *p) {
+        return delete static_cast<char*>(p);
+    }
+};
+
 int main(int, char**)
 {
     static_assert(( std::is_convertible<std::shared_ptr<A>, std::shared_ptr<B> >::value), "");
     static_assert((!std::is_convertible<std::shared_ptr<B>, std::shared_ptr<A> >::value), "");
     static_assert((!std::is_convertible<std::shared_ptr<A>, std::shared_ptr<C> >::value), "");
     {
         const std::shared_ptr<A> pA(new A);
         assert(pA.use_count() == 1);
assert(A::count == 0);
         }
         assert(pA.use_count() == 0);
         assert(B::count == 0);
         assert(A::count == 0);
     }
     assert(B::count == 0);
     assert(A::count == 0);
 
-  return 0;
+    // This should work in C++03 but we get errors when trying to do SFINAE with the delete operator.
+#if TEST_STD_VER >= 11
+    {
+        // Make sure that when T (for std::shared_ptr<T>) is an array type, this constructor only
+        // participates in overload resolution when `delete[] p` is well formed. And when T is not
+        // an array type, this constructor only participates in overload resolution when `delete p`
+        // is well formed.
+        static_assert(!std::is_constructible<std::shared_ptr<private_delete_op>,
+                                                             private_delete_op*>::value, "");
+        static_assert(!std::is_constructible<std::shared_ptr<private_delete_arr_op[4]>,
+                                                             private_delete_arr_op*>::value, "");
+    }
+#endif // STD_TEST_VERSION >= 11
 }