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