[PATCH] D150525: [libc++][memory] P1132R8: out_ptr - a scalable output pointer abstraction

[Nikolas Klauser via Phabricator via llvm-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__memory/inout_ptr.h:34
+  static_assert(!__is_specialization_v<_Smart, shared_ptr>, "std::shared_ptr<> is not supported");
+
+public:
----------------
Maybe also `static_assert` Cpp17NullablePointer?


================
Comment at: libcxx/include/__memory/inout_ptr.h:38-41
+          if constexpr (requires { __s.get(); }) {
+            return __s.get();
+          } else {
+            return __s;
----------------
AFAICT this should be `if constexpr (is_pointer_v<_Smart>)`. Why do you check whether `s.get()` is valid?


================
Comment at: libcxx/include/__memory/inout_ptr.h:62
+          [&](auto&&... __args) {
+            __s_ = _Smart{static_cast<_SP>(__p_), std::forward<_Args>(__args)...};
+          },
----------------
This shouldn't use curly braces. Otherwise an implicit conversion is impossible. regression test!


================
Comment at: libcxx/include/__memory/inout_ptr.h:71
+          [&](auto&&... __args) {
+            __s_ = _Smart{static_cast<_SP>(__p_), std::forward<_Args>(__args)...};
+          },
----------------
Same here.


================
Comment at: libcxx/include/__memory/inout_ptr.h:95-99
+  if constexpr (!is_void_v<_Pointer>) {
+    return std::inout_ptr_t<_Smart, _Pointer, _Args&&...>{__s, std::forward<_Args>(__args)...};
+  } else {
+    return std::inout_ptr_t<_Smart, std::__pointer_of_t<_Smart>, _Args&&...>{__s, std::forward<_Args>(__args)...};
+  }
----------------



================
Comment at: libcxx/include/__memory/out_ptr.h:32-34
+  static_assert(!__is_specialization_v<_Smart, shared_ptr> ||
+                    (__is_specialization_v<_Smart, shared_ptr> && sizeof...(_Args) > 0),
+                "Specialization of std::shared_ptr<> requires a deleter.");
----------------



================
Comment at: libcxx/include/__memory/out_ptr.h:38
+  _LIBCPP_HIDE_FROM_ABI explicit out_ptr_t(_Smart& __s, _Args... __args)
+      : __s_{__s}, __a_{std::forward<_Args>(__args)...} {
+    if constexpr (requires { __s.reset(); }) {
----------------
Looks like you're not value-initializing `__p_`.


================
Comment at: libcxx/include/__memory/out_ptr.h:42
+    } else {
+      static_assert(is_constructible_v<_Smart>);
+      __s_ = _Smart{};
----------------
Isn't the assertion a bit redundant?


================
Comment at: libcxx/include/__memory/out_ptr.h:64
+          std::move(__a_));
+    };
+  }
----------------
This isn't ill-formed in the else case!


================
Comment at: libcxx/include/__memory/out_ptr.h:85-89
+  if constexpr (!is_void_v<_Pointer>) {
+    return std::out_ptr_t<_Smart, _Pointer, _Args&&...>(__s, std::forward<_Args>(__args)...);
+  } else {
+    return std::out_ptr_t<_Smart, std::__pointer_of_t<_Smart>, _Args&&...>(__s, std::forward<_Args>(__args)...);
+  }
----------------



================
Comment at: libcxx/include/__memory/pointer_traits.h:270
+  __s.reset(static_cast<__pointer_of_or_t<_Smart, _Pointer>>(__p), std::forward<_Args>(__args)...);
+};
+
----------------
Why is this implemented completely differently from the standard wording?


================
Comment at: libcxx/test/std/utilities/smartptr/adapt/inout.ptr/inout.ptr.compile.fail.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
This should be a `.verify.cpp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150525