[libcxx-commits] [PATCH] D131372: Implement std::variant::operator<=>

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this!



================
Comment at: libcxx/include/__variant/monostate.h:33
+
+inline _LIBCPP_INLINE_VISIBILITY
+constexpr strong_ordering operator<=>(monostate, monostate) noexcept { return strong_ordering::equal; }
----------------
`consexpr` already implies `inline`.


================
Comment at: libcxx/include/variant:184
   constexpr bool operator==(monostate, monostate) noexcept;
-  constexpr bool operator!=(monostate, monostate) noexcept;
+  constexpr bool operator!=(monostate, monostate) noexcept;             // until C++20
+  constexpr bool operator<(monostate, monostate) noexcept;              // until C++20
----------------
we typically use `remove in` instead of `until` (like cppreference).


================
Comment at: libcxx/include/variant:1644
+template <class... _Types> requires (three_way_comparable<_Types> && ...)
+inline _LIBCPP_INLINE_VISIBILITY constexpr
+common_comparison_category_t<compare_three_way_result_t<_Types>...>
----------------
`inline` isn't needed here, both a template and constexpr imply that.


================
Comment at: libcxx/include/variant:1648
+            const variant<_Types...>& __rhs) {
+  using __variant_detail::__visitation::__variant;
+  using __result_t = common_comparison_category_t<compare_three_way_result_t<_Types>...>;
----------------
please use clang-format on this new code.


================
Comment at: libcxx/include/variant:1654
+  if (auto __c = __lhs.index() <=> __rhs.index(); __c != 0) return __c;
+  auto __three_way = []<class _Type>(const _Type& __v, const _Type& __w) -> __result_t {
+    return __v <=> __w;
----------------
Why a separate lambda instead of a lambda in the visitor?


================
Comment at: libcxx/test/std/utilities/variant/variant.monostate.relops/relops.pass.cpp:26
+#if TEST_STD_VER > 17
+#include <compare>
+#endif // TEST_STD_VER > 17
----------------
avogelsgesang wrote:
> afaik `<variant>` has a standard-mandated include to `<compare`. Hence, this include shouldn't be necessary
As a side note, you can just include headers unconditionally, they will be installed even when their contents in unavailable in a language version. (There are some exceptions, which apply when language features are disabled, like no localization available.)


================
Comment at: libcxx/test/std/utilities/variant/variant.monostate.relops/relops.pass.cpp:30
 int main(int, char**) {
   using M = std::monostate;
   constexpr M m1{};
----------------
Can you improve this test and the next by using the comparison macros and using our current constexpr test pattern

```
int main(int, const char**) {
  test();
  static_assert(test());

  return 0;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131372