[libcxx-commits] [PATCH] D103478: [libc++][compare] Implement three_way_comparable[_with] concepts

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp:67
+static_assert(std::three_way_comparable<unsigned long const volatile&&, std::strong_ordering>);
+static_assert(std::three_way_comparable<unsigned long const volatile&&, std::weak_ordering>);
+
----------------
As usual, I //don't// think it's important to have hundreds of lines of similar/redundant test cases, and I //do// think it's important to have simple test cases for the corner cases:

- What if the type provides `<=>` but no viable `==`? And vice versa?
- What if the type provides `<=>` with a non-comparison-category return type, such as `int`?
- What if the type provides `==` with a non-boolean return type, such as `int`? such as `void`?
- What if the type provides `<=>` but it works only on non-const objects?
- What if the type provides all six C++17 comparison operators but no C++20 `<=>`?
etc.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable_with.compile.pass.cpp:76-79
+static_assert(
+    !check_three_way_comparable_with<int, int (S::*)() const volatile>());
+static_assert(!check_three_way_comparable_with<
+              int, int (S::*)() const volatile noexcept>());
----------------
Nit: Please avoid taking clang-format's advice in cases like this where it's clearly making the formatting worse.
Instead, try treating a clang-format diagnostic as a helpful hint that your lines may be too long. E.g. in this case you don't //really// need to test a `const volatile noexcept` abominable function type, right? This test clearly could be rewritten as `static_assert(!check_three_way_comparable_with<int, int*>())` (or any similar pair of types; the only important thing here is that the expression `a == b` would be ill-formed).
Renaming `check_three_way_comparable_with` to simply `check` is another good way to improve the readability of this test file while also helping to make clang-format happy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103478