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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb 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>);
+
----------------
Quuxplusone wrote:
> 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.
As usual, the number of tests here are to ensure that all the sharp objects C++ has to offer are checked. I'll say it once again: concepts are a very new feature, and are relatively untested. Our spaceship operator is a a very new feature, and is relatively untested. As far as I'm aware, LLVM has zero other tests that check these two features interact. These tests ensure that nothing has been accidentally overlooked, and are a cheap way for the whole LLVM project to ensure everything is working as intended.


================
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>());
----------------
Quuxplusone wrote:
> 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.
I'm sure @rarutyun's employer doesn't pay them to spend time overriding clang-format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103478