[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
Mon Jul 20 23:30:32 PDT 2020
abrachet added inline comments.
================
Comment at: libc/utils/UnitTest/Test.cpp:40
+ cpp::EnableIfType<cpp::IsIntegral<T>::Value, int> N = sizeof(T) * 2>
+std::string uintToHex(T X, bool LowerCase = false) {
+ std::string s(N, '0');
----------------
sivachandra wrote:
> Nit: Do we need the flexibility in case? If not, just remove it?
Is it more flexible? It's semantically the same as making this `std::string uintToHex(__uint128_t X, ...)` but inflates the binary.
================
Comment at: libc/utils/UnitTest/Test.cpp:43
+
+ for (auto it = s.rbegin(); it != s.rend(); ++it, X >>= 4) {
+ unsigned char Mod = static_cast<unsigned char>(X) & 15;
----------------
`for (auto it = s.rbegin(), end = s.rend(); it != end; ...)`
================
Comment at: libc/utils/UnitTest/Test.cpp:97
+ const char *RHSStr, const char *File, unsigned long Line,
+ const std::string &OpString) {
+ size_t OffsetLength = OpString.size() > 2 ? OpString.size() - 2 : 0;
----------------
Can this be `llvm::StringRef` instead?
================
Comment at: libc/utils/UnitTest/Test.cpp:99
+ size_t OffsetLength = OpString.size() > 2 ? OpString.size() - 2 : 0;
+ std::string Offset(' ', OffsetLength);
+
----------------
This isn't the constructor you're thinking of. You want `(OffsetLength, ' ')`
================
Comment at: libc/utils/UnitTest/Test.cpp:118
+template <typename ValType>
+cpp::EnableIfType<cpp::IsFloatingPointType<cpp::RemoveCVType<ValType>>::Value,
+ bool>
----------------
The RemoveCV isn't necessary anymore
================
Comment at: libc/utils/UnitTest/Test.cpp:136-137
Ctx.markFail();
- llvm::outs() << File << ":" << Line << ": FAILURE\n"
- << " Expected: " << LHSStr << '\n'
- << " Which is: " << LHS << '\n'
- << "To be equal to: " << RHSStr << '\n'
- << " Which is: " << RHS << '\n';
-
+ explainDifference(LHS, RHS, LHSStr, RHSStr, File, Line, "equal to");
return false;
----------------
Because LHS, RHS... are always the same maybe we can create a lambda at the top of this function called fail which just takes the `OpString`
================
Comment at: libc/utils/UnitTest/Test.cpp:154
case Cond_LE:
if (LHS <= RHS)
return true;
----------------
Maybe this and >= need to check if `testEQ(...) || LHS < RHS`, but FWIW I'm not sure the testEQ is particularly necessary in the first place.
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