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