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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this.



================
Comment at: libcxx/include/compare:487
+template<class _Tp, class _Cat>
+concept __compares_as =                 // exposition only
+  same_as<common_comparison_category_t<_Tp, _Cat>, _Cat>;
----------------
We typically omit the 'exposition only' comment here.
Can you also update the synopsis with the newly added code. When copying please make sure to remove non-ASCII characters. (The invisible hyphens come to mind.)



================
Comment at: libcxx/include/compare:494
+  __partially_ordered_with<_Tp, _Tp> &&
+  requires(__make_const_lvalue_ref<_Tp>& a, __make_const_lvalue_ref<_Tp>& b) {
+    { a <=> b } -> __compares_as<_Cat>;
----------------
Please use __ugly_names: `a` -> `__a`, `b` -> `__b`.


================
Comment at: libcxx/include/compare:507
+  __partially_ordered_with<_Tp, _Up> &&
+  requires(__make_const_lvalue_ref<_Tp>& t, __make_const_lvalue_ref<_Up>& u) {
+    { t <=> u } -> __compares_as<_Cat>;
----------------
Also __ugly_names here.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp:29
+static_assert(std::three_way_comparable<wchar_t&>);
+static_assert(std::three_way_comparable<char8_t const&>);
+static_assert(std::three_way_comparable<char16_t volatile&>);
----------------
`char8_t` isn't available on all platforms, please guard with `#ifnef _LIBCPP_HAS_NO_CHAR8_T`.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp:31
+static_assert(std::three_way_comparable<char16_t volatile&>);
+static_assert(std::three_way_comparable<char32_t const volatile&>);
+static_assert(std::three_way_comparable<unsigned char&&>);
----------------
Likewise for `char16_t` and `char32_t` using `#ifndef _LIBCPP_HAS_NO_UNICODE_CHARS`


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp:35
+static_assert(std::three_way_comparable<unsigned int volatile&&>);
+static_assert(std::three_way_comparable<unsigned long const volatile&&>);
+
----------------
To be complete it would be nice to test `__int128_t`. This should be guarded with `#ifndef _LIBCPP_HAS_NO_INT128`. `__uin128_t` uses the same guard.


================
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>);
+
----------------
cjdb wrote:
> 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.
I also like the verbose testing in this case since it's a new feature. I would like to reduce the amount of redundancy, All types here have basically the same three tests
```
// with default ordering
static_assert(std::three_way_comparable<T>);

// with explicit ordering
static_assert(std::three_way_comparable<T, std::strong_ordering>);
static_assert(std::three_way_comparable<T, std::weak_ordering>);
```
Can we move this to a template helper function?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103478