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

[Siva Chandra via Phabricator via libc-commits]

sivachandra 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');
----------------
abrachet wrote:
> 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.
Not sure I understand your comment. My comment was about `LowerCase` not being important. We want this function to be a template because it is called with values of different `T` types but the case seems not required.


================
Comment at: libc/utils/UnitTest/Test.cpp:99
+  size_t OffsetLength = OpString.size() > 2 ? OpString.size() - 2 : 0;
+  std::string Offset(' ', OffsetLength);
+
----------------
abrachet wrote:
> This isn't the constructor you're thinking of. You want `(OffsetLength, ' ')`
Ah, good catch!


================
Comment at: libc/utils/UnitTest/Test.cpp:154
   case Cond_LE:
     if (LHS <= RHS)
       return true;
----------------
abrachet wrote:
> 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.
This is a good point and I should have made some notes myself. Even I am not sure what the best approach is. But, `testEQ` is required because, as explained in the comment there, we want `+0.0` and `-0.0` to not be equal whereas machine instructions treat them as equal. Next, it is a bit of a slippery slope if we handle only the `TEST_LE|GT` cases as you suggest. For, what should `TEST_GT(+0.0, -0.0)` return? We have been treating zeros and NaNs as special inputs and testing them separately anyway. So, what you say about not requiring `testEQ` has value as the test macros will only be used with non-special values where the specialness of `testEQ` isn't required. But, will it always be the case? I am not sure what the correct answer is, but I am OK with `testEQ` as I am viewing it as "experimental". That said, I am also OK if @lntue wants to get rid of `testEQ` as you suggest. Either way, it is very easy to special case in the tests, and also to debug if something goes wrong. Going forward, I would expect us to pick the approach of least pain.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83931