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

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added a comment.

> Thanks for your contribution!

Thank you for your review!

> 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.)

I will do so. I will let you know as soon as the other reviews are updated (probably only after we land this one, so I don't have to update the other commits multiple times)



================
Comment at: libcxx/docs/ReleaseNotes.rst:44
 
+* Implemented ``operator<=>`` for ``std::shared_ptr``
+
----------------
Mordante wrote:
> We usually only mention "larger" improvements here not every subpart of a paper.
works for me - that also gets rid of the annoying merge conflict on this line which I will have with all of the other posted reviews


================
Comment at: libcxx/include/__memory/shared_ptr.h:1351
+{
+    return compare_three_way()(__a.get(), static_cast<typename shared_ptr<_Tp>::element_type*>(nullptr));
+}
----------------
Mordante wrote:
> I see this matches the Wording in the Standard, but it differs from the paper. Does this complete an LWG issue?
Indeed, this fixes https://github.com/cplusplus/draft/commit/28430c71f1f3ce39f6c9a9509756bfcece270203, also known as https://cplusplus.github.io/LWG/issue3427


================
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**)
 {
----------------
Mordante wrote:
> 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.
> 
> 
Added calls to `AssertComparisonsAreNoexcept`, `AssertComparisonsReturnBool`, `AssertOrderAreNoexcept`, etc.

My code required additional `#ifdef`s, though.
Would it make sense to introduce `AssertOrderReturn` also for dialects < C++20, and let it gracefully degrade to `AssertComparisonsReturnBool` on those earlier C++ versions?


================
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
----------------
Mordante wrote:
> Please test all equality and comparison operators.
Those were already tested in `lt.pass.cpp` and `eq.pass.cpp`.
I do agree, though, that the splitting across multiple files made it hard to judge the existing test coverage. Hence, I merged this test with the pre-existing `lt.pass.cpp` and `eq.pass.cpp`.
I think doing so improves readability of this test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130852