[libc++][LWG 2996] Implement c++20 shared_ptr rvalue overloads.

[pateldeev]

Summary

Implement c++20 shared_ptr rvalue overloads for aliasing constructor and pointer casts. See https://cplusplus.github.io/LWG/issue2996

Commit from
"patedeev" <[e-mail]>

[pateldeev]

variable name.

[pateldeev]

Unit tests.

[pateldeev]

Fix

[pateldeev]

Unit tests.

[pateldeev]

new line.

[Mordante]

Thanks for working on this. In general looks quite good, but some code uses an older style we don't use for new code.

libcxx/docs/Status/Cxx20Issues.csv

105

libcxx/include/__memory/shared_ptr.h

594

Please update the synopsis too.

594

We (and other vendors) typically backport LWG-issue, can you do that for this issue too.

596

681

Please undo this change. In the past we used _VSTD instead of std. We don't change all existing code to avoid breaking too much work in progress, but we use it when changing code.

1388

_LIBCPP_HIDE_FROM_ABI is the new name for _LIBCPP_INLINE_VISIBILITY.

1389

1405

See above.

1406

In new code we prefer using over typedef.

1441

The extra space isn't fixed by clang-format due to C++03 compatibility.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/const_pointer_cast.pass.cpp

14

Please test whether this is really noexcept. There is a ASSERT_NOEXCEPT helper macro.
The same for the other tests.

[pateldeev]

libcxx/include/__memory/shared_ptr.h

594

I think back-porting to c++17 will break code that does a std::move() but relies on the fact that no move happens. Arguable this a just a bug in user code, but probably still something we don't want to change?

[pateldeev]

Review

[pateldeev]

Review

[Mordante]

libcxx/include/__memory/shared_ptr.h

594

I don't feel that we should guard against users doing bad things; updating their code to C++20 will break that scenario.

I see MSVC STL applied this change unconditionally, but libstdc++ limits it to C++20 and newer.

@jwakely is there a reason for not backporting LWG-2996 to older versions?

594

Or remove it entirely.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/const_pointer_cast.pass.cpp

68

Or removed, the same for other tests.

[jwakely]

libcxx/include/__memory/shared_ptr.h

594

I don't remember why I only added it for C++20, but probably because it's questionable whether there was a defect. It seems more like an evolutionary change (which is why it went through LEWG) than a defect fix.

[Mordante]

libcxx/include/__memory/shared_ptr.h

594

I don't remember why I only added it for C++20, but probably because it's questionable whether there was a defect. It seems more like an evolutionary change (which is why it went through LEWG) than a defect fix.

Thanks for the information. Then it makes sense to me to do the same in libc++. @pateldeev please leave a comment why this LWG issue isn't backported to earlier language versions.

[pateldeev]

Comments

[ldionne]

@philnik Can you have a pass at this?

[philnik]

Please mention the LWG issue you are implementing in the title. Other than that and inline comments this LGTM. Leaving final approval to @Mordante, since he had comments.

libcxx/include/__memory/shared_ptr.h

596

The LWG issue is against C++20. Same for the other changes.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/const_pointer_cast.pass.cpp

68

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

115–127

I'd just remove this test. I don't really see the point of testing this, since it's questionable to rely on this behaviour and it's been changed with an LWG issue which some implementations back-port.

[pateldeev]

Review. Rebase

[ldionne]

Thanks for the change, LGTM modulo comments and green CI (please rebase first). Thanks!

libcxx/docs/Status/Cxx20Issues.csv

105

[pateldeev]

Rebase

[pateldeev]

Rebased diff and addressed all comments. Should be ready to merge. But it seems I don't have the ability to do this.

@ldionne: Can you help merge this?

[Mordante]

In D135548#4134054, @pateldeev wrote:

Rebased diff and addressed all comments. Should be ready to merge. But it seems I don't have the ability to do this.

@ldionne: Can you help merge this?

Could you rebased this again and upload it to test the CI.
Can you also provide "user name" <[e-mail]> then we can commit it on your behalf.

[pateldeev]

Rebase

[pateldeev]

Could you rebased this again and upload it to test the CI.

Done

Can you also provide "user name" <[e-mail]> then we can commit it on your behalf.

Added to bottom of diff description

[Mordante]

Can you rebase this patch to see whether it passes the CI?

[pateldeev]

Rebase

[pateldeev]

cc @Mordante to merge.

[Mordante]

In D135548#4507586, @pateldeev wrote:

cc @Mordante to merge.

Thanks I can now pick it without conflicts. I noticed the attribution is "pateldeev <[e-mail]>" Can you provide they name you want me to attribute this patch too or confirm "pateldeev" is intended?

[pateldeev]

In D135548#4511146, @Mordante wrote:

In D135548#4507586, @pateldeev wrote:

cc @Mordante to merge.

Thanks I can now pick it without conflicts. I noticed the attribution is "pateldeev <[e-mail]>" Can you provide they name you want me to attribute this patch too or confirm "pateldeev" is intended?

That attribution works. No need to update. Thanks

[Mordante]

In D135548#4511151, @pateldeev wrote:

In D135548#4511146, @Mordante wrote:

In D135548#4507586, @pateldeev wrote:

cc @Mordante to merge.

Thanks I can now pick it without conflicts. I noticed the attribution is "pateldeev <[e-mail]>" Can you provide they name you want me to attribute this patch too or confirm "pateldeev" is intended?

That attribution works. No need to update. Thanks

Thanks, I'll land it on your behalf.

libcxx/docs/Status/Cxx20Issues.csv

 "`2499 <https://wg21.link/LWG2499>`__","``operator>>(basic_istream&, CharT*)``\  makes it hard to avoid buffer overflows","San Diego","Resolved by P0487R1",""
 "`2682 <https://wg21.link/LWG2682>`__","``filesystem::copy()``\  won't create a symlink to a directory","San Diego","|Nothing To Do|",""
 "`2697 <https://wg21.link/LWG2697>`__","[concurr.ts] Behavior of ``future/shared_future``\  unwrapping constructor when given an invalid ``future``\ ","San Diego","",""
 "`2797 <https://wg21.link/LWG2797>`__","Trait precondition violations","San Diego","Resolved by 1285R0",""
 "`2936 <https://wg21.link/LWG2936>`__","Path comparison is defined in terms of the generic format","San Diego","|Complete|",""
 "`2943 <https://wg21.link/LWG2943>`__","Problematic specification of the wide version of ``basic_filebuf::open``\ ","San Diego","|Nothing To Do|",""
 "`2960 <https://wg21.link/LWG2960>`__","[fund.ts.v3] ``nonesuch``\  is insufficiently useless","San Diego","|Complete|",""
 "`2995 <https://wg21.link/LWG2995>`__","``basic_stringbuf``\  default constructor forbids it from using SSO capacity","San Diego","",""
-"`2996 <https://wg21.link/LWG2996>`__","Missing rvalue overloads for ``shared_ptr``\  operations","San Diego","",""
+"`2996 <https://wg21.link/LWG2996>`__","Missing rvalue overloads for ``shared_ptr``\  operations","San Diego","|Complete|",""
 "`3008 <https://wg21.link/LWG3008>`__","``make_shared``\  (sub)object destruction semantics are not specified","San Diego","",""
 "`3022 <https://wg21.link/LWG3022>`__","``is_convertible<derived*, base*>``\  may lead to ODR","San Diego","Resolved by 1285R0",""
 "`3025 <https://wg21.link/LWG3025>`__","Map-like container deduction guides should use ``pair<Key, T>``\ , not ``pair<const Key, T>``\ ","San Diego","|Complete|",""
 "`3031 <https://wg21.link/LWG3031>`__","Algorithms and predicates with non-const reference arguments","San Diego","",""
 "`3037 <https://wg21.link/LWG3037>`__","``polymorphic_allocator``\  and incomplete types","San Diego","",""
 "`3038 <https://wg21.link/LWG3038>`__","``polymorphic_allocator::allocate``\  should not allow integer overflow to create vulnerabilities","San Diego","|Complete|","14.0"
 "`3054 <https://wg21.link/LWG3054>`__","``uninitialized_copy``\  appears to not be able to meet its exception-safety guarantee","San Diego","|Nothing To Do|",""
 "`3065 <https://wg21.link/LWG3065>`__","LWG 2989 missed that all ``path``\ 's other operators should be hidden friends as well","San Diego","|Complete|",""

libcxx/include/__memory/shared_ptr.h

#endif // _LIBCPP_NO_EXCEPTIONS
     shared_ptr(const shared_ptr<_Yp>& __r, element_type *__p) _NOEXCEPT
         : __ptr_(__p),
           __cntrl_(__r.__cntrl_)
     {
         if (__cntrl_)
             __cntrl_->__add_shared();
     }
 
+#if _LIBCPP_STD_VER > 20
+    template <class _Yp>
+    _LIBCPP_HIDE_FROM_ABI shared_ptr(shared_ptr<_Yp>&& __r, element_type* __p) _NOEXCEPT
+        : __ptr_(__p),
+          __cntrl_(__r.__cntrl_) {
+      __r.__ptr_   = nullptr;
+      __r.__cntrl_ = nullptr;
+    }
+#endif
+
     _LIBCPP_HIDE_FROM_ABI
     shared_ptr(const shared_ptr& __r) _NOEXCEPT
         : __ptr_(__r.__ptr_),
           __cntrl_(__r.__cntrl_)
     {
         if (__cntrl_)
             __cntrl_->__add_shared();
     }

 #if _LIBCPP_STD_VER > 11
         if (__ptr_ == nullptr)
             __cntrl_ = nullptr;
         else
 #endif
         {
             typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
             typedef __shared_ptr_pointer<typename unique_ptr<_Yp, _Dp>::pointer, _Dp, _AllocT> _CntrlBlk;
-            __cntrl_ = new _CntrlBlk(__r.get(), std::move(__r.get_deleter()), _AllocT());
+            __cntrl_ = new _CntrlBlk(__r.get(), _VSTD::move(__r.get_deleter()), _AllocT());
             __enable_weak_this(__r.get(), __r.get());
         }
         __r.release();
     }
 
     template <class _Yp, class _Dp, class = void, class = __enable_if_t<
         is_lvalue_reference<_Dp>::value &&
         is_convertible<typename unique_ptr<_Yp, _Dp>::pointer, element_type*>::value

 shared_ptr<_Tp>
 static_pointer_cast(const shared_ptr<_Up>& __r) _NOEXCEPT
 {
     return shared_ptr<_Tp>(__r,
                            static_cast<
                                typename shared_ptr<_Tp>::element_type*>(__r.get()));
 }
 
+#if _LIBCPP_STD_VER > 20
+template <class _Tp, class _Up>
+inline _LIBCPP_INLINE_VISIBILITY shared_ptr<_Tp> static_pointer_cast(shared_ptr<_Up>&& __r) _NOEXCEPT {
+  return shared_ptr<_Tp>(_VSTD::move(__r), static_cast<typename shared_ptr<_Tp>::element_type*>(__r.get()));
+}
+#endif
+
 template<class _Tp, class _Up>
 inline _LIBCPP_INLINE_VISIBILITY
 shared_ptr<_Tp>
 dynamic_pointer_cast(const shared_ptr<_Up>& __r) _NOEXCEPT
 {
     typedef typename shared_ptr<_Tp>::element_type _ET;
     _ET* __p = dynamic_cast<_ET*>(__r.get());
     return __p ? shared_ptr<_Tp>(__r, __p) : shared_ptr<_Tp>();
 }
 
+#if _LIBCPP_STD_VER > 20
+template <class _Tp, class _Up>
+inline _LIBCPP_INLINE_VISIBILITY shared_ptr<_Tp> dynamic_pointer_cast(shared_ptr<_Up>&& __r) _NOEXCEPT {
+  typedef typename shared_ptr<_Tp>::element_type _ET;
+  _ET* __p = dynamic_cast<_ET*>(__r.get());
+  return __p ? shared_ptr<_Tp>(_VSTD::move(__r), __p) : shared_ptr<_Tp>();
+}
+#endif
+
 template<class _Tp, class _Up>
 _LIBCPP_HIDE_FROM_ABI shared_ptr<_Tp>
 const_pointer_cast(const shared_ptr<_Up>& __r) _NOEXCEPT
 {
     typedef typename shared_ptr<_Tp>::element_type _RTp;
     return shared_ptr<_Tp>(__r, const_cast<_RTp*>(__r.get()));
 }
 
+#if _LIBCPP_STD_VER > 20
+template <class _Tp, class _Up>
+_LIBCPP_HIDE_FROM_ABI shared_ptr<_Tp> const_pointer_cast(shared_ptr<_Up>&& __r) _NOEXCEPT {
+  typedef typename shared_ptr<_Tp>::element_type _RTp;
+  return shared_ptr<_Tp>(_VSTD::move(__r), const_cast<_RTp*>(__r.get()));
+}
+
+#endif
+
 template<class _Tp, class _Up>
 _LIBCPP_HIDE_FROM_ABI shared_ptr<_Tp>
 reinterpret_pointer_cast(const shared_ptr<_Up>& __r) _NOEXCEPT
 {
     return shared_ptr<_Tp>(__r,
                            reinterpret_cast<
                                typename shared_ptr<_Tp>::element_type*>(__r.get()));
 }
 
+#if _LIBCPP_STD_VER > 20
+template <class _Tp, class _Up>
+_LIBCPP_HIDE_FROM_ABI shared_ptr<_Tp> reinterpret_pointer_cast(shared_ptr<_Up>&& __r) _NOEXCEPT {
+  return shared_ptr<_Tp>(_VSTD::move(__r), reinterpret_cast< typename shared_ptr<_Tp>::element_type*>(__r.get()));
+}
+#endif
+
 #ifndef _LIBCPP_NO_RTTI
 
 template<class _Dp, class _Tp>
 inline _LIBCPP_INLINE_VISIBILITY
 _Dp*
 get_deleter(const shared_ptr<_Tp>& __p) _NOEXCEPT
 {
     return __p.template __get_deleter<_Dp>();

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/const_pointer_cast.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>
 
 // shared_ptr
 
 // template<class T, class U> shared_ptr<T> const_pointer_cast(const shared_ptr<U>& r);
+// template<class T, class U> shared_ptr<T> const_pointer_cast(shared_ptr<U>&& r);
 
+#include <cassert>
 #include <memory>
 #include <type_traits>
-#include <cassert>
+#include <utility>
 
 #include "test_macros.h"
 
 struct B
 {
     static int count;
 
     B() {++count;}

 #if TEST_STD_VER > 14
     {
       const std::shared_ptr<const A[8]> pA;
       std::shared_ptr<A[8]> pB = std::const_pointer_cast<A[8]>(pA);
       assert(pB.get() == pA.get());
       assert(!pB.owner_before(pA) && !pA.owner_before(pB));
     }
 #endif // TEST_STD_VER > 14
+#if TEST_STD_VER > 20
+    {
+      A* pA_raw = new A;
+      std::shared_ptr<const A> pA(pA_raw);
+      std::shared_ptr<A> pB = std::const_pointer_cast<A>(std::move(pA));
+      assert(pA.get() == nullptr);
+      assert(pB.get() == pA_raw);
+      assert(pB.use_count() == 1);
+    }
+#endif // TEST_STD_VER > 20
 
     return 0;
 }

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/dynamic_pointer_cast.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>
 
 // shared_ptr
 
 // template<class T, class U> shared_ptr<T> dynamic_pointer_cast(const shared_ptr<U>& r);
+// template<class T, class U> shared_ptr<T> dynamic_pointer_cast(shared_ptr<U>&& r);
 
 // UNSUPPORTED: no-rtti
 
+#include <cassert>
 #include <memory>
 #include <type_traits>
-#include <cassert>
+#include <utility>
 
 #include "test_macros.h"
 
 struct B
 {
     static int count;
 
     B() {++count;}

 #if TEST_STD_VER > 14
     {
       const std::shared_ptr<B[8]> pB(new B[8]);
       std::shared_ptr<A[8]> pA = std::dynamic_pointer_cast<A[8]>(pB);
       assert(pA.get() == 0);
       assert(pA.use_count() == 0);
     }
 #endif // TEST_STD_VER > 14
+#if TEST_STD_VER > 20
+    {
+      A* pA_raw = new A;
+      std::shared_ptr<B> pB(pA_raw);
+      std::shared_ptr<A> pA = std::dynamic_pointer_cast<A>(std::move(pB));
+      assert(pB.get() == nullptr);
+      assert(pA.get() == pA_raw);
+      assert(pA.use_count() == 1);
+    }
+#endif // TEST_STD_VER > 20
 
     return 0;
 }

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/reinterpret_pointer_cast.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>
 
 // shared_ptr
 
 // template<class T, class U>
 //     shared_ptr<T> reinterpret_pointer_cast(const shared_ptr<U>& r) noexcept;
+// template<class T, class U>
+//     shared_ptr<T> reinterpret_pointer_cast(shared_ptr<U>&& r) noexcept;
 
 #include "test_macros.h"
 
+#include <cassert>
 #include <memory>
 #include <type_traits>
-#include <cassert>
+#include <utility>
 
 struct A {
   int x;
 };
 
 struct Base { };
 struct Derived : public Base { };
 
#if TEST_STD_VER > 14
   {
     const std::shared_ptr<A[8]> pA;
     std::shared_ptr<int[8]> pi = std::reinterpret_pointer_cast<int[8]>(pA);
     std::shared_ptr<A[8]> pA2 = std::reinterpret_pointer_cast<A[8]>(pi);
     assert(pA2.get() == pA.get());
     assert(!pi.owner_before(pA) && !pA.owner_before(pi));
   }
 #endif // TEST_STD_VER > 14
+#if TEST_STD_VER > 20
+  {
+    A* pA_raw = new A;
+    std::shared_ptr<A> pA(pA_raw);
+    std::shared_ptr<int> pi = std::reinterpret_pointer_cast<int>(std::move(pA));
+    assert(pA.get() == nullptr);
+    assert(pi.use_count() == 1);
+    std::shared_ptr<A> pA2 = std::reinterpret_pointer_cast<A>(std::move(pi));
+    assert(pi.get() == nullptr);
+    assert(pA2.get() == pA_raw);
+    assert(pA2.use_count() == 1);
+  }
+#endif // TEST_STD_VER > 20
 
   return 0;
 }

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/static_pointer_cast.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>
 
 // shared_ptr
 
 // template<class T, class U> shared_ptr<T> static_pointer_cast(const shared_ptr<U>& r);
+// template<class T, class U> shared_ptr<T> static_pointer_cast(shared_ptr<U>&& r);
 
+#include <cassert>
 #include <memory>
 #include <type_traits>
-#include <cassert>
+#include <utility>
 
 #include "test_macros.h"
 
 struct B
 {
     static int count;
 
     B() {++count;}
#if TEST_STD_VER > 14
     }
     {
       const std::shared_ptr<B[8]> pA;
       std::shared_ptr<A[8]> pB = std::static_pointer_cast<A[8]>(pA);
       assert(pB.get() == pA.get());
       assert(!pB.owner_before(pA) && !pA.owner_before(pB));
     }
 #endif // TEST_STD_VER > 14
+#if TEST_STD_VER > 20
+    {
+      A* pA_raw = new A;
+      std::shared_ptr<A> pA(pA_raw);
+      std::shared_ptr<B> pB = std::static_pointer_cast<B>(std::move(pA));
+      assert(pA.get() == nullptr);
+      assert(pB.get() == pA_raw);
+      assert(pB.use_count() == 1);
+    }
+#endif // TEST_STD_VER > 20
 
     return 0;
 }

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

int main(int, char**)
     delete pi;
     {
       std::shared_ptr<int> p1(new int);
       std::shared_ptr<int> p2(p1, nullptr);
       assert(p2.get() == nullptr);
     }
 
 #if TEST_STD_VER > 17 && defined(_LIBCPP_VERSION)
-    // This won't pass when LWG-2996 is implemented.
     {
       std::shared_ptr<A> pA(new A);
       assert(pA.use_count() == 1);
 
+#  if TEST_STD_VER > 20
+      // LWG-2996 is implemented in c++20 and beyond.
+      {
+        B b;
+        std::shared_ptr<B> pB(std::move(pA), &b);
+        assert(A::count == 1);
+        assert(B::count == 1);
+        assert(pA.use_count() == 0);
+        assert(pB.use_count() == 1);
+        assert(pB.get() == &b);
+      }
+#  else
       {
         B b;
         std::shared_ptr<B> pB(std::move(pA), &b);
         assert(A::count == 1);
         assert(B::count == 1);
         assert(pA.use_count() == 2);
         assert(pB.use_count() == 2);
         assert(pB.get() == &b);
       }
       assert(pA.use_count() == 1);
       assert(A::count == 1);
       assert(B::count == 0);
+#  endif // TEST_STD_VER > 20
     }
     assert(A::count == 0);
     assert(B::count == 0);
 #endif
 
     return 0;
 }