[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:2638
+  // type.
+  QualType getCorrespondingSignedFixedPointType(QualType Ty) const;
+
----------------
Please include in the comment here that, unlike `getCorrespondingUnsignedType`, this has to be specific to fixed-point types because there are unsigned integer types like `bool` and `char8_t` that don't have signed equivalents.


================
Comment at: clang/include/clang/Basic/FixedPoint.h:41
+      assert(!HasUnsignedPadding &&
+             "Cannot have unsigned padding on a signed type.");
   }
----------------
`assert(!(IsSigned && HasUnsignedPadding) && ...);`, please.


================
Comment at: clang/include/clang/Basic/FixedPoint.h:67
+  FixedPointSemantics
+  getCommonSemantics(const FixedPointSemantics &Other) const;
+
----------------
Actually representing the fully-precise value is operation-specific; you'd need a different calculation for at least addition and multiplication, and maybe also subtraction and (maybe?) division.  Is this what you actually want?


================
Comment at: clang/include/clang/Basic/FixedPoint.h:70
+  /// Return the FixedPointSemantics for an integer type.
+  static FixedPointSemantics GetIntegerSemantics(unsigned Width, bool IsSigned);
+
----------------
This is trivial enough that it should probably be implemented inline.


================
Comment at: clang/lib/AST/ASTContext.cpp:10489
+    return FixedPointSemantics::GetIntegerSemantics(getIntWidth(Ty),
+                                                    Ty->isSignedIntegerType());
+
----------------
Does `getIntWidth` do the thing you want for `bool`?


================
Comment at: clang/lib/Basic/FixedPoint.cpp:141
+  if (ResultIsSigned || ResultHasUnsignedPadding)
+    CommonWidth++;
+
----------------
Is this right?  If I have `a+b+c+d+e+f+g+h+i`, where those are all the same signed type, won't the width of the common semantics keep growing as I add more operands?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53738/new/

https://reviews.llvm.org/D53738