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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks! Looks quite good overall, but a few remarks.



================
Comment at: libcxx/include/__memory/unique_ptr.h:601
+                                   typename unique_ptr<_T2, _D2>::pointer>
+compare_three_way_result_t<typename unique_ptr<_T1, _D1>::pointer,
+                           typename unique_ptr<_T2, _D2>::pointer>
----------------



================
Comment at: libcxx/include/__memory/unique_ptr.h:711
+requires three_way_comparable<typename unique_ptr<_T1, _D1>::pointer>
+compare_three_way_result_t<typename unique_ptr<_T1, _D1>::pointer>
+operator<=>(const unique_ptr<_T1, _D1>& __x, nullptr_t) {
----------------



================
Comment at: libcxx/include/memory:509
+    bool operator!=(const unique_ptr<T1, D1>& x, const unique_ptr<T2, D2>& y);    // removed in C++20
 template <class T1, class D1, class T2, class D2>
     bool operator<(const unique_ptr<T1, D1>& x, const unique_ptr<T2, D2>& y);
----------------
Interesting these aren't removed, including in the WP.


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.special/cmp.pass.cpp:72
+#if TEST_STD_VER > 17
+  AssertOrderReturn<std::strong_ordering, std::unique_ptr<int>>();
+#endif
----------------
Can you also test with the other two comparison categories? Not 100% sure it's even possible.


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.special/cmp_nullptr.pass.cpp:22
 // template <class T, class D>
-//     bool operator<(const unique_ptr<T, D>& x, nullptr_t) noexcept;
+//     bool operator<(const unique_ptr<T, D>& x, nullptr_t);
 // template <class T, class D>
----------------
Nice catch! It seems we've added `_NOEXCEPT` in the code. @ldionne should we remove these `noexcept`s in a separate commit?


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.special/cmp_nullptr.pass.cpp:55
+#if TEST_STD_VER > 17
+  AssertOrderReturn<std::strong_ordering, std::unique_ptr<int>>();
+#endif
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130838