[libc-commits] [PATCH] D136799: [libc] Implement a high-precision floating point class.

[Michael Jones via Phabricator via libc-commits]

michaelrj added a comment.

Overall this looks promising, but I'd like to see some unit tests for the big float class if possible.



================
Comment at: libc/src/__support/FPUtil/big_float.h:46
+    sign = a_bits.get_sign();
+    exponent = a_bits.get_exponent() - 52;
+    mantissa = mantissa_type(a_bits.get_explicit_mantissa());
----------------
this could probably be templated by using `FloatProperties<T>::MantissaWidth`


================
Comment at: libc/src/__support/FPUtil/big_float.h:75
+  // to be normals.
+  double to_double() const {
+    if (is_zero())
----------------
same as above, I think you can make this either an operator or a templated `to_float<T>`


================
Comment at: libc/src/__support/FPUtil/big_float.h:87
+      m <<= shift;
+      e -= shift;
+    }
----------------
did you mean to have this twice?


================
Comment at: libc/src/__support/FPUtil/big_float.h:209
+#endif // LLVM_LIBC_SRC_SUPPORT_FPUTIL_BIG_FLOAT_H
\ No newline at end of file

----------------
nit: please fix


================
Comment at: libc/src/__support/UInt.h:25-28
+template <typename T> struct pair {
+  T lo = 0;
+  T hi = 0;
+};
----------------
The `pair` class should probably be in a separate file.


================
Comment at: libc/src/__support/UInt.h:659
 // Provides is_integral of UInt<128>.
 template <> struct is_integral<UInt<128>> : public cpp::true_type {};
+template <> struct is_integral<UInt<192>> : public cpp::true_type {};
----------------
is it possible to have this as <T> instead of specifying `128`, `196`, and `256`? Same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136799