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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik accepted this revision.
philnik added a comment.
This revision is now accepted and ready to land.

In D146392#4301291 <https://reviews.llvm.org/D146392#4301291>, @H-G-Hristov wrote:

> Ping @philnik
>
> The CI issue is unrelated.
>
> Sorry if personal pings are unacceptable!

That's fine (and even important in some cases), especially if the person requested changes.

LGTM with the last comment addressed.



================
Comment at: libcxx/test/std/utilities/optional/optional.comp_with_t/compare.three_way.pass.cpp:34-75
+  friend constexpr SomeInt& operator+=(SomeInt& lhs, const SomeInt& rhs) {
+    lhs.value_ += rhs.value_;
+    return lhs;
+  }
+  friend constexpr SomeInt& operator-=(SomeInt& lhs, const SomeInt& rhs) {
+    lhs.value_ -= rhs.value_;
+    return lhs;
----------------
I don't think any of this is necessary. You don't have to make it work like an int to make it comparable to an int.


================
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};
----------------
philnik wrote:
> Please also add tests for a user-defined type that is comparable to `int`.
I wanted you to test that it doesn't convert 'U' to 'T' :D (The other tests are also nice though)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146392