bricci added a comment.
Added some comments inline.
However I have not looked at the logic too closely and
have not looked at the tests at all.
================
Comment at: cfe/trunk/include/clang/AST/ASTContext.h:1966
unsigned char getFixedPointIBits(QualType Ty) const;
+ FixedPointSemantics getFixedPointSemantics(QualType Ty) const;
+ APFixedPoint getFixedPointMax(QualType Ty) const;
----------------
could you add some comments for these functions ?
What are they doing ?
================
Comment at: cfe/trunk/include/clang/Basic/FixedPoint.h:24
+
+class ASTContext;
+class QualType;
----------------
This should be removed if unused. It is very strange to have
something basic like a fixed point type depending on ASTContext...
================
Comment at: cfe/trunk/include/clang/Basic/FixedPoint.h:31
+/// When HasUnsignedPadding is true and this type is signed, the first bit
+/// in the value this represents is treaded as padding.
+class FixedPointSemantics {
----------------
s/treaded/treated
================
Comment at: cfe/trunk/include/clang/Basic/FixedPoint.h:54
+ return Width - Scale;
+ }
+
----------------
Do you really need the branch here?
Why not just return
`Width - Scale - (IsSigned || (!IsSigned && HasUnsignedPadding))`
Also doc
================
Comment at: cfe/trunk/include/clang/Basic/FixedPoint.h:62
+ bool HasUnsignedPadding;
+};
+
----------------
My point is that you have two unsigned and then 3 bools.
This means that your class has now sizeof == 3*sizeof(unsigned)
This is annoying on 64 bits archs since pretty much everything
in clang is aligned to 8 bytes (because of pointer alignment requirements)
Just stuff your bits into Width or Scale. Moreover I think that
you could just use a single unsigned to store all of this.
For example you could here use a bit-field like so:
unsigned Width : 15; (or any other appropriate split with Scale)
unsigned Scale : 14; (or any other appropriate split with Width)
unsigned IsSigned : 1;
unsigned IsSaturated : 1;
unsigned HasUnsignedPadding : 1;
Now I do not now if this matters or not in this case but it seems
to me that this is easy to do.
================
Comment at: cfe/trunk/include/clang/Basic/FixedPoint.h:85
+ Sema) {}
+
+ llvm::APSInt getValue() const { return llvm::APSInt(Val, !Sema.isSigned());
}
----------------
same comment about "sema"
================
Comment at: cfe/trunk/include/clang/Basic/FixedPoint.h:99
+ }
+
+ APFixedPoint shl(unsigned Amt) const {
----------------
Maybe a bit more documentation here...
Also use /// for comments explaining something to an user:
eg:
/// Create a new Frob blablabla
Frob makeFrobinator()
so that doxygen picks up these comments.
Use // for comments which are internal only
eg:
...
// TODO this is broken because blablabla
...
================
Comment at: cfe/trunk/include/clang/Basic/FixedPoint.h:110
+ }
+
+ // If LHS > RHS, return 1. If LHS == RHS, return 0. If LHS < RHS, return -1.
----------------
doc
================
Comment at: cfe/trunk/include/clang/Basic/FixedPoint.h:120
+ bool operator>(const APFixedPoint &Other) const { return compare(Other) > 0;
}
+ bool operator<(const APFixedPoint &Other) const { return compare(Other) < 0;
}
+ bool operator>=(const APFixedPoint &Other) const {
----------------
does this fit into 80 cols ?
================
Comment at: cfe/trunk/include/clang/Basic/FixedPoint.h:130
+ static APFixedPoint getMin(const FixedPointSemantics &Sema);
+
+private:
----------------
doc
================
Comment at: cfe/trunk/lib/AST/ASTContext.cpp:10439
+FixedPointSemantics ASTContext::getFixedPointSemantics(QualType Ty) const {
+ assert(Ty->isFixedPointType());
+ bool isSigned = Ty->isSignedFixedPointType();
----------------
assert( ... && "Ty is not a fixed point type!")
================
Comment at: cfe/trunk/lib/AST/ASTContext.cpp:10448
+APFixedPoint ASTContext::getFixedPointMax(QualType Ty) const {
+ assert(Ty->isFixedPointType());
+ return APFixedPoint::getMax(getFixedPointSemantics(Ty));
----------------
same
================
Comment at: cfe/trunk/lib/AST/ASTContext.cpp:10453
+APFixedPoint ASTContext::getFixedPointMin(QualType Ty) const {
+ assert(Ty->isFixedPointType());
+ return APFixedPoint::getMin(getFixedPointSemantics(Ty));
----------------
same
================
Comment at: cfe/trunk/lib/Basic/FixedPoint.cpp:15
+
+#include "clang/AST/ASTContext.h"
+#include "clang/Basic/FixedPoint.h"
----------------
Basic depends on ASTContext ? huh
================
Comment at: cfe/trunk/lib/Basic/FixedPoint.cpp:98
+ }
+
+ return 0;
----------------
Would this work: do a test for equality and return 0 if true,
then it seems to me that you can drop every `else if`.
Also maybe use the above trick to remove some branches.
Repository:
rL LLVM
https://reviews.llvm.org/D48661
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
