[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/Basic/FixedPoint.h:23
+
+class FixedPointNumber {
+ public:
----------------
ebevhan wrote:
> rjmccall wrote:
> > The established naming convention here — as seen in `APInt`, `APFloat`, `APValue`, etc. — would call this `APFixedPoint`.  Maybe that's not a great convention, but we should at least discuss deviating from it.
> > 
> > You might also want a type which encapsulates the details of a fixed-point type, i.e. the semantic width, scale, and saturating-ness.  (Like the "float semantics" of `APFloat`.)
> > 
> > I think a key question here is whether you want a FixedPointNumber to exactly represent the bit-pattern or just the semantic value.  I think it would eliminate a non-trivial source of bugs in this type if it just represents the semantic value, i.e. if a 16-bit unsigned fract value on a target where that uses a padded representation did not explicitly represent the padding bit, and then just added it back in in some accessor that asks for the bit-pattern.  Regardless, that should be clearly documented.
> >  a 16-bit unsigned fract value on a target where that uses a padded representation did not explicitly represent the padding bit
> 
> So does that mean that the underlying APInt in this type would be 15 bits instead of 16, to avoid representing the padding? It feels a bit scary to throw around values with different internal representation than what other parts of the code (say, the target specification) have specified them to be.
Well, you can encapsulate it so that the 15-bit APInt is never exposed to users of the type.  That's a relatively small number (probably just 1) of places to check for correctness, vs. having to have logic to handle the padding bit in basically every operation on the type in addition to all the other configuration-explosions that are actually mandatory.


Repository:
  rC Clang

https://reviews.llvm.org/D48661