[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:325-326
+{
+  using T1U = make_unsigned_t<_Tp>;
+  using T2U = make_unsigned_t<_Up>;
+  if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
----------------
curdeius wrote:
> Please _Uglify the names.
Thanks for renaming `T1` to `_Tp` and `T2` to `_Up`; however, that implies you also need to rename `T2U` into e.g. `_UnsignedUp`. Or just get rid of the name; it seems like you use it only once? E.g.
```
if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
   return __t == __u;
else if constexpr (is_signed_v<_Tp>)
   return (__t >= 0) && (__u == make_unsigned_t<_Tp>(__t));
else
   return (__u >= 0) && (__t == make_unsigned_t<_Up>(__u));
```
I'm actually puzzled how this isn't equivalent to simply
```
if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
   return __t == __u;
else
   return (__t >= 0) && (__u >= 0) && (__t == __u);
```
but I don't //object// to the longer form, especially since it's currently given on cppreference.


================
Comment at: libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_equal/cmp_equal.pass.cpp:96-112
+  using Types1 =
+      std::tuple<signed char, unsigned char, int8_t, uint8_t, short,
+                 unsigned short, int16_t, uint16_t, int, unsigned int, int32_t,
+                 uint32_t, long, unsigned long, int64_t, uint64_t, long long,
+                 unsigned long long
+#ifndef _LIBCPP_HAS_NO_INT128
+                 ,
----------------
It appears that `Types1` and `Types2` are just the exact same list in a different order.
So I think what you want to do here is use just one list, but do all O(n^2) comparisons:
```
std::tuple<
#ifndef _LIBCPP_HAS_NO_INT128
    __int128_t, __uint128_t,
#endif
    unsigned long long, long long, unsigned long, long,
    unsigned int, int, unsigned short, short,
    unsigned char, signed char
> types;
test1(types);
test2(types, types);
```
preceded by
```
template <class... Ts>
constexpr void test1(const std::tuple<Ts...>&) {
  assert((test_cmp_equal1<Ts>() && ...));
}
template <class T, class... Us>
constexpr void test2_impl(const std::tuple<Us...>& utuple) {
    assert((test_cmp_equal2<T, Us>() && ...));
}
template <class... Ts, class UTuple>
constexpr void test2(const std::tuple<Ts...>&, const UTuple& utuple) {
    (test2_impl<Ts>(utuple) , ...);
}
```

You don't need to test `uint32_t` etc. because those are just library typedefs for the primitive types that you're already testing (e.g. `unsigned int` or `unsigned long`).


================
Comment at: libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_greater_equal/cmp_greater_equal.pass.cpp:56-63
+  if constexpr (std::is_signed_v<T>) {
+    assert(std::cmp_greater_equal(T(-1), T(-1)));
+
+    assert(std::cmp_greater_equal(-2, tup.min));
+    assert(!std::cmp_greater_equal(tup.min, -2));
+    assert(!std::cmp_greater_equal(-2, tup.max));
+    assert(std::cmp_greater_equal(tup.max, -2));
----------------
I think it'd probably make sense to rewrite this as unconditional — lose the `if`:

    assert(std::cmp_greater_equal(T(-1), T(-1)));
    assert(std::cmp_greater_equal(-2, tup.min) == std::is_signed_v<T>);
    assert(std::cmp_greater_equal(tup.min, -2) == std::is_unsigned_v<T>);
    assert(!std::cmp_greater_equal(-2, tup.max));
    assert(std::cmp_greater_equal(tup.max, -2));


================
Comment at: libcxx/test/std/utilities/utility/utility.intcmp/intcmp.cmp_greater_equal/cmp_greater_equal.pass.cpp:113-114
+
+  ASSERT_NOEXCEPT(
+      std::cmp_greater_equal(std::declval<int>(), std::declval<int>()));
+  test();
----------------
Spurious newline here.


================
Comment at: libcxx/test/std/utilities/utility/utility.intcmp/intcmp.fail.cpp:55-56
+      in_range<T>(int());
+  std:: // expected-error11{{no matching function for call to 'in_range'}}
+      in_range<int>(T());
+}
----------------
This style was... not what I wanted. :P  I can fix the style post-commit, if need be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94511