[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]

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