[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 ↗

(On Diff #466430)

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 ↗

(On Diff #466430)

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

69 ↗

(On Diff #467324)

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

69 ↗

(On Diff #468702)

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 ↗

(On Diff #489319)

[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/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

 #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(), _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

 {
     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()));
+  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 {
+_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 {
+_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*

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

#if TEST_STD_VER > 14
     }
 #endif // TEST_STD_VER > 14
 #if TEST_STD_VER > 20
     {
       A* pA_raw = new A;
       std::shared_ptr<A> pA(new A);
       std::shared_ptr<B> pB = std::static_pointer_cast<B>(std::move(pA));
       assert(pA.get() == nullptr);
-      assert(pB.get() == pA_raw);
+      assert(pB.get() == static_cast<B*>(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;
 }