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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
