[libcxx-commits] [PATCH] D135548: [libc++] Implement c++20 shared_ptr rvalue overloads.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante requested changes to this revision.
Mordante added a comment.
This revision now requires changes to proceed.

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



================
Comment at: libcxx/docs/Status/Cxx20Issues.csv:105
 "`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","",""
----------------



================
Comment at: libcxx/include/__memory/shared_ptr.h:594
 
+#if _LIBCPP_STD_VER > 20
+    template <class _Yp>
----------------
Please update the synopsis too.


================
Comment at: libcxx/include/__memory/shared_ptr.h:594
 
+#if _LIBCPP_STD_VER > 20
+    template <class _Yp>
----------------
Mordante wrote:
> Please update the synopsis too.
We (and other vendors) typically backport LWG-issue, can you do that for this issue too.


================
Comment at: libcxx/include/__memory/shared_ptr.h:596
+    template <class _Yp>
+    _LIBCPP_HIDE_FROM_ABI shared_ptr(shared_ptr<_Yp>&& __r, element_type* __p) _NOEXCEPT
+        : __ptr_(__p),
----------------



================
Comment at: libcxx/include/__memory/shared_ptr.h:681
             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());
----------------
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.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1388
+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()));
----------------
`_LIBCPP_HIDE_FROM_ABI` is the new name for `_LIBCPP_INLINE_VISIBILITY`.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1389
+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()));
+}
----------------



================
Comment at: libcxx/include/__memory/shared_ptr.h:1405
+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;
----------------
See above.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1406
+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());
----------------
In new code we prefer `using` over `typedef`.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1440
+_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()));
+}
----------------
The extra space isn't fixed by clang-format due to C++03 compatibility.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cast/const_pointer_cast.pass.cpp:14
 // 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);
 
----------------
Please test whether this is really noexcept. There is a `ASSERT_NOEXCEPT` helper macro.
The same for the other tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135548/new/

https://reviews.llvm.org/D135548