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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates added inline comments.


================
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>...>
----------------
Mordante wrote:
> `inline` isn't needed here, both a template and constexpr imply that.
Should `inline` therefore be removed from all of these operators?


================
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>...>;
----------------
Mordante wrote:
> please use clang-format on this new code.
just for this function? i can't seem to get my editor to run clang-format only on selected code, so it's quite painful. i'm also not sure if the result really helps readability here, it just shuffles some linebreaks around.

are there specific formatting rules you'd like to be honored?


================
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;
----------------
Mordante wrote:
> Why a separate lambda instead of a lambda in the visitor?
To keep line length and readability under control, mostly, same reason we added `using` for the return type.


================
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{};
----------------
Mordante wrote:
> 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;
> }
> ```
done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131372