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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates added inline comments.


================
Comment at: libcxx/include/variant:1651
+  using __result_t = common_comparison_category_t<compare_three_way_result_t<_Types>...>;
+  if (__lhs.valueless_by_exception() && __rhs.valueless_by_exception()) return strong_ordering::equal;
+  if (__lhs.valueless_by_exception()) return strong_ordering::less;
----------------
Mordante wrote:
> The `return` is normally on its own line.
> 
> FYI `git clang-format HEAD^` can be used to format the changes in the last commit and then you can amend the wanted changes. This is basically what I do to format only relevant changes.
`git clang-format HEAD^` does not seem to actually change anything in `<variant>` even when i squash all the changes in this differential into a single commit. it does clean up a couple things in some of the other files, though. likewise `git clang-format main` does nothing once those changes are applied; it never decides to act on `<variant>`.

i let it format that function manually (even though i'm not really on board with un-braced controlled statements on a different line) and normalized all the indentation to the trailing backslashes in multiline macros to the old 80 column limit. but, i still don't know why it chose not to touch this file.


================
Comment at: libcxx/test/std/utilities/variant/variant.monostate.relops/relops.pass.cpp:30
   constexpr M m2{};
-  {
-    static_assert((m1 < m2) == false, "");
-    ASSERT_NOEXCEPT(m1 < m2);
-  }
-  {
-    static_assert((m1 > m2) == false, "");
-    ASSERT_NOEXCEPT(m1 > m2);
-  }
-  {
-    static_assert((m1 <= m2) == true, "");
-    ASSERT_NOEXCEPT(m1 <= m2);
-  }
-  {
-    static_assert((m1 >= m2) == true, "");
-    ASSERT_NOEXCEPT(m1 >= m2);
-  }
-  {
-    static_assert((m1 == m2) == true, "");
-    ASSERT_NOEXCEPT(m1 == m2);
-  }
-  {
-    static_assert((m1 != m2) == false, "");
-    ASSERT_NOEXCEPT(m1 != m2);
-  }
+  testComparisons(m1, m2, /*isEqual*/ true, /*isLess*/ false);
+  AssertComparisonsAreNoexcept<M>();
----------------
avogelsgesang wrote:
> `testComparisons` returns a bool indicating if it was succesful or not. The way you wrote this test case, it does not provide the intended test coverage.
> The correct way to write this is
> 
> ```
> assert(testComparisons(m1, m2, /*isEqual*/ true, /*isLess*/ false));
> ```
> 
> Here and below in line 34
> 
> Also see https://reviews.llvm.org/D131364 and consider giving me a review on that
dang you're right, i saw that differential and didn't remember it here. it's a good change


================
Comment at: libcxx/test/std/utilities/variant/variant.relops/three_way.pass.cpp:18
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
----------------
Mordante wrote:
> Can you move this to line 8?
sure!


================
Comment at: libcxx/test/std/utilities/variant/variant.relops/three_way.pass.cpp:73
+    V v1;
+    makeEmpty(v1);
+    assert(testOrder(v1, v2, std::weak_ordering::equivalent));
----------------
Mordante wrote:
> Why switch the order of V1 and V2?
great question, not sure how that happened


================
Comment at: libcxx/test/std/utilities/variant/variant.relops/three_way.pass.cpp:106
+    assert(testOrder(v1, v2, Order(std::strong_ordering::greater)));
+  }
+
----------------
Mordante wrote:
> Can you add a test for `std::partial_ordering::unordered`?
yes! doing so revealed a core deficiency in the `test_comparisons.h` helper header as well, so i expanded upon that file to support testing unordered values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131372