[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:31
+  SatNoPadding,
+};
+
----------------
leonardchan wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > I figured you'd want this to be a struct which include the scale, width, signed-ness, and saturating-ness; and then `APFixedPoint` can just store one of these next to a bare `APInt`.
> > That's what I was expecting too, similar to the APFloat version.
> > 
> > What width would it contain? The width with or without padding? Probably the one with padding as the APInt itself has the width without padding.
> Woops my bad, I thought you were referring to the `APFloatSemantics`. I actually didn't know about `fltSemantics` until now.
Thanks, this looks a lot better to me.  Please do capitalize `FixedPointSemantics`, though; `fltSemantics` is a bizarre deviation from LLVM's style guidelines that should be fixed, not something to emulate.


================
Comment at: include/clang/Basic/FixedPoint.h:41
+  APFixedPoint(const llvm::APSInt &Val, unsigned Scale,
+               enum FixedPointSemantics Sema)
+      : Val(Val), Scale(Scale), Sema(Sema) {}
----------------
rjmccall wrote:
> Why the elaborated-type-specifier?
Similarly, this should be renamed to `getIntegralBits` to follow the normal LLVM method-naming guidelines.

Also, please go ahead and hide all the members here behind getters and setters.  It's better to reserve flexibility in advance than to have to rewrite a bunch of code later.


Repository:
  rC Clang

https://reviews.llvm.org/D48661