[libc-commits] [PATCH] D105153: [libc] Add on float properties for precision floating point numbers in FloatProperties.h

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/utils/FPUtil/FloatProperties.h:34
 
   // If a number x is a NAN, then it is a quiet NAN if:
   //   QuietNaNMask & bits(x) != 0
----------------
The quiet NAN masks are probably incorrect. What is a quiet NAN and what is a signalling NAN depends on the target architecture. I will remove them in a separate patch. In the meanwhile, do not add them to the new structs you are defining.


================
Comment at: libc/utils/FPUtil/FloatProperties.h:61
+// Properties for long double on Windows platform, same as double precision.
+#if defined(LONG_DOUBLE_IS_DOUBLE)
+template <> struct FloatProperties<long double> {
----------------
A high level comment: These `#if defined` clauses should be mutually exclusive. As in, the next one in the series should be `#elif defined`, and the last one `#else`.


================
Comment at: libc/utils/FPUtil/FloatProperties.h:85
+  static constexpr BitsType quietNaNMask =
+      FloatProperties<double>::quietNaNMask;
+};
----------------
As pointed out above, do not add this member.


================
Comment at: libc/utils/FPUtil/FloatProperties.h:107
+  // Else, it is a signalling NAN.
+  static constexpr BitsType quietNaNMask = 0x8000000000000000U;
+};
----------------
As pointed out above, do not add this member.


================
Comment at: libc/utils/FPUtil/FloatProperties.h:123
+  static constexpr BitsType mantissaMask =
+      ((BitsType)0x0000ffffffffffffU << 64) | (BitsType)0xffffffffffffffffU;
+  static constexpr BitsType signMask = (BitsType)0x8000000000000000U << 64;
----------------
Here and elsewhere in this patch, a more C++ way to do the compile time cast/conversion is to do it this way:

```
BitsType(...) << 64 | BitsType(...)
```

As opposed to the C style casts you have. The above style is probably also more readable.


================
Comment at: libc/utils/FPUtil/FloatProperties.h:133
+      (BitsType)0x0000800000000000U
+      << 64; // Has the most significant bit in the mantissa set to one.
+};
----------------
As pointed out above, do not add this member.


================
Comment at: libc/utils/FPUtil/FloatProperties.h:140
 
 template <> struct FloatType<uint32_t> {
   static_assert(sizeof(uint32_t) == sizeof(float),
----------------
These should also go. So, do not add the new class below.


================
Comment at: libc/utils/FPUtil/FloatProperties.h:152
 
+template <> struct FloatType<__uint128_t> {
+  static_assert(sizeof(__uint128_t) == sizeof(long double),
----------------
As pointed out above, do not add this new class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105153