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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/utils/CPP/TypeTraits.h:57
+
+template <typename T> using RemoveCVType = typename RemoveCV<T>::Type;
+
----------------
abrachet wrote:
> I believe that `typename` shouldn't be required and `RemoveCV<T>::Type` should be enough.
`RemoveCV<T>::Type` is a dependent type and should require `typename`? I tried your suggestion and that is what the compiler also tells me:

```
TypeTraits.h:57:44: error: missing 'typename' prior to dependent type name 'RemoveCV<T>::Type'
```


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:29-41
+  template <typename XType,
+            cpp::EnableIfType<cpp::IsSame<float, XType>::Value, int> = 0>
+  explicit MPFRNumber(XType x) {
     mpfr_init2(value, mpfrPrecision);
     mpfr_set_flt(value, x, MPFR_RNDN);
   }
 
----------------
abrachet wrote:
> Do these need to be enable if specializations could they just be overloads?
Yes. Having explicit enables avoids implicit conversions of the arguments. Implicit conversions can potentially lead of loss of precision. Added a  comment saying the same.


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:43-44
+
+  template <typename XType,
+            cpp::EnableIfType<cpp::IsFloatingPointType<XType>::Value, int> = 0>
+  MPFRNumber(Operation op, XType rawValue) {
----------------
abrachet wrote:
> Could this also be just a static_assert?
Again, are you suggesting to put a `static_assert` inside the function body? If yes, it feels a bit odd to error-out with a custom message rather than give a normal compiler message for a constructor.


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.h:75-77
+template <typename T,
+          __llvm_libc::cpp::EnableIfType<
+              __llvm_libc::cpp::IsFloatingPointType<T>::Value, int> = 0>
----------------
abrachet wrote:
> Can this one also be `static_assert`?
Done, but I assumed you meant we should instead put a `static_assert` inside the function body? I think that makes sense. But, see my comment at the other place. I don't have any strong preference, but I do see that in this case the `static_assert` inside the function body is much nicer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79256