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

[Bruno Ricci via Phabricator via cfe-commits]

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