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

John McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jul 3 18:23:06 PDT 2018

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.

  rC Clang


More information about the cfe-commits mailing list