[libc-commits] [PATCH] D105561: [libc] (WIP) Modify the struct that captures floating point encoding to have setters and getters

[Arthur Eubanks via Phabricator via libc-commits]

aeubanks added a comment.

the title should say something about creating a custom packed int, the getter and setter part is a small detail that shouldn't be in the title



================
Comment at: libc/utils/FPUtil/FPBits.h:23
 
 template <typename T> struct MantissaWidth {};
 template <> struct MantissaWidth<float> {
----------------
since we're basically just forwarding MantissaWidth to FloatProperties::mantissaWidth, we can delete all the specializations below and do
```
template <typename T> struct MantissaWidth {
    static constexpr unsigned value = FloatProperties<T>::mantissaWidth;
};
```

Or even better, delete these and and only use FloatProperties::mantissaWidth. But maybe that can be done in a follow up patch.


================
Comment at: libc/utils/FPUtil/FPBits.h:83
+  struct {
+    using FloatProp = typename __llvm_libc::fputil::FloatProperties<T>;
+
----------------
I think we don't need the `__llvm_libc::fputil::` since we're arleady inside that namespace


================
Comment at: libc/utils/FPUtil/FPBits.h:86
+  private:
+    UIntType valueFP;
+
----------------
I don't really like `valueFP`, how about something like `bits`


================
Comment at: libc/utils/FPUtil/FPBits.h:92
+
+    // Clean and check the value given with its mask so it does not lead to
+    // overflow. Then invert the bit mask to clear the old value.
----------------
these comments should probably be deleted, they're not super useful


================
Comment at: libc/utils/FPUtil/FPBits.h:117
+      return uint16_t((valueFP & FloatProp::exponentMask) >>
+                      (FloatProp::bitWidth - 1 - FloatProp::exponentWidth));
+    }
----------------
this is very confusing, can we just use `mantissaWidth`?


================
Comment at: libc/utils/FPUtil/FPBits.h:129
+    bool getSign() const {
+      return uint8_t((valueFP & FloatProp::signMask) >>
+                     (FloatProp::bitWidth - 1));
----------------
`bool`?


================
Comment at: libc/utils/FPUtil/FPBits.h:134
   } encoding;
   UIntType integer;
   T val;
----------------
we should replace all uses of `integer` with `encoding.valueFP`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105561