[libcxx-commits] [PATCH] D130852: [libc++] Implement `operator<=>` for `shared_ptr`

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for your contribution! When resolving the review comments please look whether they apply to your other patches too. (I first want to look at this patch, once approved I'll look at the others.)

For the CI, feel free to ignore the error of the format CI, this is a soft-error at the moment.



================
Comment at: libcxx/docs/ReleaseNotes.rst:44
 
+* Implemented ``operator<=>`` for ``std::shared_ptr``
+
----------------
We usually only mention "larger" improvements here not every subpart of a paper.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1239
+template<class _Tp, class _Up>
+strong_ordering
+operator<=>(shared_ptr<_Tp> const& __a, shared_ptr<_Up> const& __b) _NOEXCEPT
----------------



================
Comment at: libcxx/include/__memory/shared_ptr.h:1240
+strong_ordering
+operator<=>(shared_ptr<_Tp> const& __a, shared_ptr<_Up> const& __b) _NOEXCEPT
+{
----------------
The macro only needs to be used in code that works in C++03.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1240
+strong_ordering
+operator<=>(shared_ptr<_Tp> const& __a, shared_ptr<_Up> const& __b) _NOEXCEPT
+{
----------------
Mordante wrote:
> The macro only needs to be used in code that works in C++03.
please use `__x` and `__y` for consistency.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1351
+{
+    return compare_three_way()(__a.get(), static_cast<typename shared_ptr<_Tp>::element_type*>(nullptr));
+}
----------------
I see this matches the Wording in the Standard, but it differs from the paper. Does this complete an LWG issue?


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp:47
 
 int main(int, char**)
 {
----------------
Partly pre-existing, but I miss tests to validate the return type and that the function is `noexcept`. Can you add tests for this? See D128603 for examples.




================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/order.pass.cpp:8-9
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
----------------



================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/order.pass.cpp:24
+int main(int, char**) {
+#if TEST_STD_VER > 17
+  int* ptr1(new int);
----------------
Please remove this `#if` the UNSUPPORTED is the libc++ way to achieve this behaviour.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/order.pass.cpp:32
+  assert((p1 <=> p2) == (ptr1 <=> ptr2));
+  assert((p2 <=> p3) == std::strong_ordering::equal);
+#endif
----------------
Please test all equality and comparison operators.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130852