[libc-commits] [PATCH] D83931: Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double).
Alex Brachet via Phabricator via libc-commits
libc-commits at lists.llvm.org
Thu Jul 16 01:30:43 PDT 2020
abrachet added a comment.
Why add support to the TEST macros for this when we have the MPFR macros which I think handle floats better than we can with TEST. Comparisons with floating points are not fun, its why we use the mpfr library in the first place. Is there even a need to use the TEST macros with floats?
================
Comment at: libc/utils/CPP/TypeTraits.h:42
template <> struct IsIntegral<bool> : public TrueValue {};
+template <> struct IsIntegral<__uint128_t> : public TrueValue {};
----------------
It's not your fault, but now that I look at these again, these are wrong. Well at least not exactly analogous to `std::is_integral`. We need to `RemoveCV` first. Might not make sense in this patch, but something to note.
================
Comment at: libc/utils/UnitTest/Test.cpp:37
+// When the value is not floating-point type, just display it as normal
+template <typename ValType>
----------------
Missing trailing . and bellow too
================
Comment at: libc/utils/UnitTest/Test.cpp:66
+ const std::string &OpString) {
+ const size_t OffsetLength = OpString.size() > 2 ? OpString.size() - 2 : 0;
+ std::string Offset(' ', OffsetLength);
----------------
This const isn't useful
================
Comment at: libc/utils/UnitTest/Test.h:81-83
+ cpp::EnableIfType<cpp::IsIntegral<ValType>::Value ||
+ cpp::IsFloatingPointType<ValType>::Value,
+ int> = 0>
----------------
This is fine because we only use it once, but this is just `std::is_arithmetic`, we could create a `IsArithmetic` if you plan to use this pattern more.
================
Comment at: libc/utils/testutils/StreamWrapper.cpp:45
+template <>
+StreamWrapper &StreamWrapper::operator<<<__uint128_t>(__uint128_t t) {
+ assert(OS);
----------------
This intuitively doesn't make sense. It's unexpected that a [u]int128 would be printed differently than the smaller width integral type. I don't know what you're trying to do with `__uint128_t` but I think it's best to create a separate type than do this.
================
Comment at: libc/utils/testutils/StreamWrapper.cpp:49
+
+ const __uint128_t chunk = 10'000'000'000'000ULL;
+ uint64_t bottom = static_cast<uint64_t>(t % chunk);
----------------
I don't think `ULL` is necessary. This could also be `uint64_t` and we could skip the static_casts.
Why `const` was it supposed to be `constexpr`?
================
Comment at: libc/utils/testutils/StringExtras.h:19-22
+template <typename T,
+ typename __llvm_libc::cpp::EnableIfType<
+ __llvm_libc::cpp::IsSame<T, __uint128_t>::Value, int> = 0>
+std::string utohexstr(T X, bool LowerCase = false) {
----------------
Why not just `std::string utohexstr(__uint128_t X, ...)`? Otherwise shouldn't this be called u128tohexstr :)
================
Comment at: libc/utils/testutils/StringExtras.h:29
+
+ while (X) {
+ unsigned char Mod = static_cast<unsigned char>(X) & 15;
----------------
`for (; X; X >>= 4)`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83931/new/
https://reviews.llvm.org/D83931
More information about the libc-commits
mailing list