[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/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:
> 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.
I also use this to display float, double, and exponent fields.  I can move the length N to a function parameter instead to reduce the generated binary.


================
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');
----------------
lntue wrote:
> sivachandra wrote:
> > 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.
> I also use this to display float, double, and exponent fields.  I can move the length N to a function parameter instead to reduce the generated binary.
Removed the case.


================
Comment at: libc/utils/UnitTest/Test.cpp:99
+  size_t OffsetLength = OpString.size() > 2 ? OpString.size() - 2 : 0;
+  std::string Offset(' ', OffsetLength);
+
----------------
sivachandra wrote:
> abrachet wrote:
> > This isn't the constructor you're thinking of. You want `(OffsetLength, ' ')`
> Ah, good catch!
Actually it still works correctly (I deliberately failed a test to see the outputs).  I think it was matched with std::string(const char*, size_t, allocator) constructor instead (constructor (4) in https://en.cppreference.com/w/cpp/string/basic_string/basic_string).  I change to the other constructor anyway.


================
Comment at: libc/utils/UnitTest/Test.cpp:154
   case Cond_LE:
     if (LHS <= RHS)
       return true;
----------------
sivachandra wrote:
> 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.
> 
I would say when we use TEST_LE|GT, we wouldn't care about the difference between +0 and -0.  So in that case, the default behavior works fine.  But we can update later if a new behavior is needed.


================
Comment at: libc/utils/testutils/StreamWrapper.cpp:46
 template StreamWrapper &StreamWrapper::operator<<<std::string>(std::string t);
+template StreamWrapper &
+    StreamWrapper::operator<<<llvm::StringRef>(llvm::StringRef t);
----------------
sivachandra wrote:
> Do we still need this for this change? May be, but just asking because I couldn't easily locate where.
No, it's not needed anymore.  Removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83931