[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/include/__compare/three_way_comparable.h:27
+template<class _Tp, class _Cat>
+concept __compares_as =                 // exposition only
+  same_as<common_comparison_category_t<_Tp, _Cat>, _Cat>;
----------------
This comment is unnecessary.


================
Comment at: libcxx/include/__compare/three_way_comparable.h:34-35
+  __partially_ordered_with<_Tp, _Tp> &&
+  requires(__make_const_lvalue_ref<_Tp> __a, __make_const_lvalue_ref<_Tp> __b) {
+    { __a <=> __b } -> __compares_as<_Cat>;
+  };
----------------
Quuxplusone wrote:
> ```
> requires(__make_const_lvalue_ref<_Tp> __t) {
>     { __t <=> __t } -> __compares_as<_Cat>;
> ```
> Let's keep it simple (and close to the reference implementation).
We shouldn't deviate from what's in the standard. The concept is defined the way it is way for a reason.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable_with.compile.pass.cpp:49
+static_assert(check_three_way_comparable_with<int, char>());
+static_assert(check_three_way_comparable_with<int, wchar_t>());
+static_assert(check_three_way_comparable_with<int, double>());
----------------
Quuxplusone wrote:
> This specific line is failing on AArch64 and Arm. I suspect that those platforms have a 32-bit unsigned `wchar_t`, and so this line would be failing for the same reason that
> ```
> static_assert(std::three_way_comparable<int, unsigned int>);
> ```
> would fail. Therefore, the correct action is to remove this platform-dependent line (and lines 55 and 59 too). However, consider adding
> ```
> static_assert(!std::three_way_comparable<int, unsigned int>);
> ```
> as a sanity check; I think you're missing anything along those lines at the moment.
Interesting. I'd prefer to section this test off using an include guard, if it's possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103478