[libcxx-commits] [PATCH] D94511: [libc++] [C++20] [P0586] Implement safe integral comparisons
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Apr 7 11:01:07 PDT 2021
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
More information about the libcxx-commits
mailing list