[libcxx-commits] [PATCH] D146392: [libc++][spaceship] Implement `operator<=>` for `optional`

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/docs/Status/SpaceshipProjects.csv:25
 | `[optional.comp.with.t] <https://wg21.link/optional.comp.with.t>`_","| `optional <https://reviews.llvm.org/D146392>`_
-| `nullopt <https://reviews.llvm.org/D146392>`_",None,Hristo Hristov,|In Progress|
+| `nullopt <https://reviews.llvm.org/D146392>`_",None,Hristo Hristov,|Complete|
 "| `[variant.relops] <https://wg21.link/variant.relops>`_
----------------
It looks like this patch implements LWG3566. If that's the case, please mark it as completed.


================
Comment at: libcxx/include/optional:1322
+    return __x.has_value() <=> __y.has_value();
+}
+
----------------
Please clang-format the new code.


================
Comment at: libcxx/include/optional:1425
 
+#elif _LIBCPP_STD_VER >= 20
+
----------------
This should just be an `#else` AFAICT.


================
Comment at: libcxx/include/optional:1585
+template<class _Tp, class _Up>
+    requires (!__is_std_optional<_Up>::value) && three_way_comparable_with<_Tp, _Up>
+_LIBCPP_HIDE_FROM_ABI
----------------
Please implement [LWG3746](http://wg21.link/LWG3746).


================
Comment at: libcxx/test/std/utilities/optional/optional.comp_with_t/compare.three_way.pass.cpp:25
+#include "test_comparisons.h"
+
+constexpr bool test() {
----------------
Please add a SFINAE test for the `three_way_comparable_with` constraint.


================
Comment at: libcxx/test/std/utilities/optional/optional.comp_with_t/compare.three_way.pass.cpp:28
+  {
+    int t{3};
+    std::optional<int> op{3};
----------------
Please also add tests for a user-defined type that is comparable to `int`.


================
Comment at: libcxx/test/std/utilities/optional/optional.relops/compare.three_way.pass.cpp:26-29
+  std::optional<int> op;
+
+  assert((op <=> op) == std::strong_ordering::equal);
+  assert(testOrder(op, op, std::strong_ordering::equal));
----------------
Also, let's use two `optional`s to make sure it's not comparing addresses or something like that.


================
Comment at: libcxx/test/std/utilities/optional/optional.relops/compare.three_way.pass.cpp:30
+  assert(testOrder(op, op, std::strong_ordering::equal));
+
+  {
----------------
Please also add tests where one of the `optional`s has a value (both left and right).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146392