[libc-commits] [PATCH] D105561: [libc] Creating a struct that captures floating point encoding and manually arranges it sequentially in memory

[Arthur Eubanks via Phabricator via libc-commits]

aeubanks added a comment.

the commit message is a bit verbose:

- no need to say that the patch was fixed
- no need to say that you updated all getters/setters of exponent/mantissa/sign, that is a direct and obvious consequence of FPBits work
- no need to say that we're using FloatProperties.h, that's obvious if you look at the patch
- the ultimate reason for this patch isn't that static_asserts are failing, it's that __attribute__((packed)) isn't portable and doesn't work on Windows

in general the commit message should be more of a "why" rather than a "what are the steps I took to create this patch"; try to figure out exactly what a reader of this patch needs to know and don't over-explain details since that clutters the message
if something tricky is being done then that should be explained of course, but most of this patch is self-explanatory if you know the reasoning behind the change



================
Comment at: libc/utils/FPUtil/FPBits.h:64
+      mantVal &= (FloatProp::mantissaMask);
+      assert((mantVal & ~(FloatProp::mantissaMask)) == 0);
+      bits &= ~(FloatProp::mantissaMask);
----------------
@sivachandra is this the right way to assert?


================
Comment at: libc/utils/FPUtil/FPBits.h:91
+
+    uint8_t getSign() const {
+      return uint8_t((bits & FloatProp::signMask) >> (FloatProp::bitWidth - 1));
----------------
not sure if this should be a bool or a uint8_t...
let's see what the others say


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105561