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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits