[libc-commits] [PATCH] D83931: Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double).

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/utils/CPP/TypeTraits.h:42
 template <> struct IsIntegral<bool> : public TrueValue {};
+template <> struct IsIntegral<__uint128_t> : public TrueValue {};
 
----------------
sivachandra wrote:
> abrachet wrote:
> > 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.
> Thanks for pointing out. I planned to fix this after adding `RemoveCVType` but it slipped. @lntue, I will fix this separately and you can take it in.
Thanks, I got Siva's updated TypeTraits.h.


================
Comment at: libc/utils/UnitTest/Test.h:81-83
+            cpp::EnableIfType<cpp::IsIntegral<ValType>::Value ||
+                                  cpp::IsFloatingPointType<ValType>::Value,
+                              int> = 0>
----------------
abrachet wrote:
> 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.
Added cpp::IsArithmetic template


================
Comment at: libc/utils/testutils/StreamWrapper.cpp:45
+template <>
+StreamWrapper &StreamWrapper::operator<<<__uint128_t>(__uint128_t t) {
+  assert(OS);
----------------
abrachet wrote:
> 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.
I needed the overload for __uint128_t because the llvm::raw_ostream does not support it.  Since Siva commented that the decimal value of __uint128_t is of no use, and the hexadecimal display of __uint128_t is moved to Test.cpp, this overload is not needed anymore.


================
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);
----------------
abrachet wrote:
> 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`?
Same as the above comment: this overload operator is not needed anymore.


================
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) {
----------------
abrachet wrote:
> Why not just `std::string utohexstr(__uint128_t X, ...)`? Otherwise shouldn't this be called u128tohexstr :)
Changed the method name to uintToHex to display a fixed number of hexadecimal digits, and moved to Test.cpp, instead of overloading llvm::utohexstr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83931