[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 a comment.

In D83931#2155091 <https://reviews.llvm.org/D83931#2155091>, @abrachet wrote:

> Why add support to the TEST macros for this when we have the MPFR macros which I think handle floats better than we can with TEST.  Comparisons with floating points are not fun, its why we use the mpfr library in the first place. Is there even a need to use the TEST macros with floats?


Let me give some explanation from my side for this. For non-trivial math functions like trignometric functions, comparing with MPFR result makes sense because we can use MPFR as source of truth/correctness. But, for functions like `fmin` and friends, comparing with MPFR is an overkill. I agree that comparing floating points in general could be problematic and that is the reason why we did not allow floating comparisons until now. But, if we restrict ourselves to values which have an exact bit representation, then comparisons are precise and deterministic. For example, zero, infinity and small decimals like 10.0, 1.2345 etc. have exact bit representation in all relevant floating point formats Likewise, if a floating point number is actually constructed from the bit representation, then that number has exact bit representation by construction. So, comparing such numbers would be precise.

One can ask, how can we ensure one uses only exact floating point numbers in tests. I don't have a good answer other than saying "code review". May be we can write a clang-tidy driven tool in future. Irrespective, I think this patch improves the readability of the tests. @lntue is the expert here so he might have more to add.



================
Comment at: libc/utils/UnitTest/Test.cpp:40
+cpp::EnableIfType<!cpp::IsFloatingPointType<ValType>::Value, void>
+describeValue(ValType Value) {
+  testutils::outs() << Value << '\n';
----------------
Can we make `describeValue` return `std::string`? This way, we will not need to overload `llvm::utohexstr`.


================
Comment at: libc/utils/UnitTest/Test.cpp:46
+template <> void describeValue<__uint128_t>(__uint128_t Value) {
+  testutils::outs() << Value << '\n'
+                    << "In hexadecimal: " << llvm::utohexstr(Value) << '\n';
----------------
I don't think its of much value to show the decimal value.


================
Comment at: libc/utils/UnitTest/Test.cpp:47
+  testutils::outs() << Value << '\n'
+                    << "In hexadecimal: " << llvm::utohexstr(Value) << '\n';
+}
----------------
So, instead of streaming the decimal value, we will just stream out the hex bits with `0x` prefix and remove the `In hexadecimal:`.  


================
Comment at: libc/utils/UnitTest/Test.cpp:56
+  fputil::FPBits<ValType> Bits(Value);
+  testutils::outs() << Value << '\n'
+                    << "Sign: " << (Bits.sign ? 1 : 0) << ", "
----------------
Same here. I think the decimal value is not of much value. May be in future, when debugging a function like `sqrt`, seeing the decimal value is useful. But, until then, I would prefer to keep the size of this patch small and skip showing decimal values.


================
Comment at: libc/utils/UnitTest/Test.cpp:59
+                    << "Exponent: " << llvm::utohexstr(Bits.exponent) << ", "
+                    << "Mantissa: " << llvm::utohexstr(Bits.exponent) << '\n';
+}
----------------
Instead of the raw value, for NaN and infinity values, I would add something like this (in parenthesis):

```
<< '(' << ["+infinity" | "- infinity" | "NaN" ] << ')' << `\n';
``` 


================
Comment at: libc/utils/UnitTest/Test.cpp:63
+template <typename ValType>
+void showDifference(ValType LHS, ValType RHS, const char *LHSStr,
+                    const char *RHSStr, const char *File, unsigned long Line,
----------------
+1 for this change. Nit: May be call the function `explainDifference`.


================
Comment at: libc/utils/testutils/StreamWrapper.cpp:1
 //===-- StreamWrapper.cpp -------------------------------------------------===//
 //
----------------
I prefer keeping this patch to the minimum required and avoid changes in this file.


================
Comment at: libc/utils/testutils/StringExtras.h:22
+              __llvm_libc::cpp::IsSame<T, __uint128_t>::Value, int> = 0>
+std::string utohexstr(T X, bool LowerCase = false) {
+  char Buffer[33];
----------------
As I have said above, can we just make `describeValue` return `std::string` and put this functionality in the `[u]int128` specialization of `describeValue`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83931