[libc-commits] [PATCH] D79256: [libc] Improve information printed on failure of a math test which uses MPFR.

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

I don't want to make any suggestions yet because I don't fully understand why we need a `MPFR_TEST`. Would you mind clarifying the need for `TEST_WITH_BASE_CLASS`? Instead of `MPFR_TEST` could we have `TEST_EXTENSION`'s first argument take a class with `compare`, would that simplify it? I'm by no means a gtest expert, I'm not sure how someone would use gtest to fill these kinds of needs. Maybe they would make their own macros which end up calling `FAIL` or using matchers. We aren't limited to that though so perhaps these would end up being more complicated than this solution.



================
Comment at: libc/utils/CPP/TypeTraits.h:50-52
+template <typename T> struct RemoveCV<const T> { typedef T type; };
+template <typename T> struct RemoveCV<volatile T> { typedef T type; };
+template <typename T> struct RemoveCV<const volatile T> { typedef T type; };
----------------
`type` -> `Type`

On that note, we could make an analog to `std::type_identity` and make this 
`template <typename T> struct RemoveCV : TypeIdentity<T> {};`. No strong preference though.


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:26-27
 
+  template <typename T>
+  friend bool internal::compare(Operation, T, T, const Tolerance &);
+
----------------
Is there a reason that this is not just a static method of `MPFRNumber`?


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:79
+    char buffer[200];
+    mpfr_sprintf(buffer, "%100.50Rf", value);
+    llvm::StringRef ref(buffer);
----------------
Might as well use `mpfr_snprintf`?


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:113-115
+    llvm::outs() << "      Input: " << mpfrInput.str() << '\n';
+    llvm::outs() << "Libc result: " << mpfrLibcResult.str() << '\n';
+    llvm::outs() << "MPFR result: " << mpfrResult.str() << '\n';
----------------
We can just keep streaming instead of ending the statement and calling `llvm::outs()` again perhaps? That would be stylistically similar to what we have in Test.cpp


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.h:56-58
+  template <typename T,
+            __llvm_libc::cpp::EnableIfType<
+                __llvm_libc::cpp::IsFloatingPointType<T>::Value, int> = 0>
----------------
We aren't enabling another specialization when this isn't true so could we just make this a `static_assert`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79256