[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

2648

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 ↗

(On Diff #337516)

Nit: spaces around =.

439 ↗

(On Diff #337516)

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 ↗

(On Diff #337516)

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 ↗

(On Diff #337516)

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/include/memory

 };
 
 #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 _Pt, class = decltype(delete _VSTD::declval<_Pt>())>
+static true_type __well_formed_delete_op_test(int);
+
+template <class>
+static false_type __well_formed_delete_op_test(...);
+
+template <class _Pt>
+struct __well_formed_delete_op : decltype(__well_formed_delete_op_test<_Pt>(0)) {};
+
+template <class _Pt, class = decltype(delete[] _VSTD::declval<_Pt>())>
+static true_type __well_formed_delete_arr_op_test(int);
+
+template <class>
+static false_type __well_formed_delete_arr_op_test(...);
+
+template <class _Pt>
+struct __well_formed_delete_arr_op : decltype(__well_formed_delete_arr_op_test<_Pt>(0)) {};
+
+template<class _Yp, class _Tp>
+struct __shared_ptr_raw_ptr_ctor_reqs
+{
+    using __delete_op_is_well_formed = typename conditional<is_array<_Tp>::value,
+                                                            __well_formed_delete_arr_op<_Yp*>,
+                                                            __well_formed_delete_op<_Yp*>>::type;
+    static const bool value = __compatible_with<_Yp, _Tp>::value &&
+                              __delete_op_is_well_formed::value;
+};
+
 template<class _Tp>
 class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _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
     typedef _Tp element_type;
 #endif
 
 private:
     element_type*      __ptr_;
     __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());
+                            typename enable_if<__shared_ptr_raw_ptr_ctor_reqs<_Yp, _Tp>::value, __nat>::type = __nat());
     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);
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)
+                            typename enable_if<__shared_ptr_raw_ptr_ctor_reqs<_Yp, _Tp>::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);

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

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // <memory>
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);
 
+// 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
+
   return 0;
 }