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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, much appreciated.  A couple more style changes that I noticed and this will be LGTM, although you should also make sure you have Bevin Hansson's approval.



================
Comment at: include/clang/AST/ASTContext.h:33
 #include "clang/Basic/AddressSpaces.h"
+#include "clang/Basic/FixedPoint.h"
 #include "clang/Basic/IdentifierTable.h"
----------------
Please forward-declare `FixedPointSemantics` and `APFixedPoint` instead of including them here.  This file is `#include`d into almost the entire project, but relatively few files will actually need to use any of the functionality of `APFixedPoint`.  Among other things, it will make your life much happier bringing up this feature if every tiny change to the `APFixedPoint` interface doesn't force a full recompile.


================
Comment at: include/clang/AST/ASTContext.h:1953
   unsigned char getFixedPointIBits(QualType Ty) const;
+  FixedPointSemantics getFixedPointSema(QualType Ty) const;
+  APFixedPoint getFixedPointMax(QualType Ty) const;
----------------
Please spell out "semantics" here.


================
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:
> 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.
All of these `inline`s are unnecessary.


Repository:
  rC Clang

https://reviews.llvm.org/D48661