[libcxx-commits] [PATCH] D94511: [libc++] [C++20] [P0586] Implement safe integral comparisons

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/utility:311
+concept __is_safe_integral_cmp = is_integral_v<_Tp> &&
+                      !__is_any_of<remove_cvref_t<_Tp>, bool, char,
+#ifndef _LIBCPP_NO_HAS_CHAR8_T
----------------
A type which is already known to be "integral" cannot also be cvref-qualified.
Please replace lines 296–318 with simply
```
template<class _Tp, class... _Up>
struct _IsSameAsAny : _Or<_IsSame<_Tp, _Up>...> {};

template<class _Tp>
concept __is_safe_integral_cmp = is_integral_v<_Tp> &&
                      !_IsSameAsAny<_Tp, bool, char,
#ifndef _LIBCPP_NO_HAS_CHAR8_T
                      char8_t,
#endif
#ifndef _LIBCPP_HAS_NO_UNICODE_CHARS
                      char16_t, char32_t,
#endif
                      wchar_t>::value;
```
(Btw I've tested this against your tests and it passes them, so if anyone finds a bug in what I wrote, then you'll need more tests.)


================
Comment at: libcxx/include/utility:349
+   return __u < 0 ? false : __t < make_unsigned_t<_Up>(__u);
+}
+
----------------
You've got 1-space indents on lines 344, 346, 348. Please make them 2-space.
Ditto lines 325, 327, 329.


================
Comment at: libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_equal/cmp_equal.pass.cpp:58-65
+  if constexpr (std::is_signed_v<T>) {
+    assert(std::cmp_equal(T(-5), T(-5)));
+
+    assert(!std::cmp_equal(-2, tup.min));
+    assert(!std::cmp_equal(tup.min, -2));
+    assert(!std::cmp_equal(-2, tup.max));
+    assert(!std::cmp_equal(tup.max, -2));
----------------
These tests also pass when `!is_signed_v<T>`, so you can remove line 58. (See also my unaddressed comment about the cmp_greater_equal test.)


================
Comment at: libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_greater/cmp_greater.pass.cpp:102
+      uint32_t, int32_t, unsigned int, int, uint16_t, int16_t, unsigned short,
+      short, uint8_t, int8_t>;
+  assert(test1(Types1{}) && test2(Types1{}, Types1{}) &&
----------------
curdeius wrote:
> Please test `signed char` and `unsigned char`. This applies to other tests too.
`std::declval<int>()` is a verbose way to write `0`. Please write `std::cmp_greater(0, 0)` or `std::cmp_greater(42, 42)`, your choice. (Ditto throughout. And then you can remove some clang-format-introduced linebreaks, too.)


================
Comment at: libcxx/test/std/utilities/utility/utility.intcmp/intcmp.in_range/in_range.pass.cpp:86
+  return 0;
+}
----------------
Bogus blank line on line 81.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94511