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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/Basic/FixedPoint.h:11
+/// \file
+/// Defines the fixed point number interface.
+//
----------------
Referencing the standard here would be good, and maybe even excerpting the key parts.


================
Comment at: include/clang/Basic/FixedPoint.h:23
+
+class FixedPointNumber {
+ public:
----------------
The established naming convention here — as seen in `APInt`, `APFloat`, `APValue`, etc. — would call this `APFixedPoint`.  Maybe that's not a great convention, but we should at least discuss deviating from it.

You might also want a type which encapsulates the details of a fixed-point type, i.e. the semantic width, scale, and saturating-ness.  (Like the "float semantics" of `APFloat`.)

I think a key question here is whether you want a FixedPointNumber to exactly represent the bit-pattern or just the semantic value.  I think it would eliminate a non-trivial source of bugs in this type if it just represents the semantic value, i.e. if a 16-bit unsigned fract value on a target where that uses a padded representation did not explicitly represent the padding bit, and then just added it back in in some accessor that asks for the bit-pattern.  Regardless, that should be clearly documented.


================
Comment at: include/clang/Basic/FixedPoint.h:26
+  FixedPointNumber(const llvm::APSInt &Val, unsigned Scale,
+                   bool Saturated = false)
+      : Val_(Val), Scale_(Scale), Saturated_(Saturated) {}
----------------
This probably shouldn't be optional, since it's a really important semantic aspect of the type whether it's saturating or not.


================
Comment at: include/clang/Basic/FixedPoint.h:83
+  unsigned Scale_;
+  bool Saturated_;
+};
----------------
LLVM convention does not include the trailing underscore; they're just capitalized.  No, I don't like it, either.


================
Comment at: include/clang/Basic/TargetInfo.h:318
+  /// Return true if unsigned fixed point types have padding for this target.
+  /// False otherwise.
+  bool unsignedFixedPointTypesHavePadding() const { return SameFBits; }
----------------
You can cut "false otherwise".


================
Comment at: include/clang/Basic/TargetInfo.h:319
+  /// False otherwise.
+  bool unsignedFixedPointTypesHavePadding() const { return SameFBits; }
+
----------------
The convention is that predicates like this start with a verb, so this should be something like `doUnsignedFixedPointTypesHavePadding`.


================
Comment at: lib/AST/ASTContext.cpp:10379
+  return 0;
+}
----------------
Please put at least the member functions on `FixedPointNumber` in their own `.cpp` file.


Repository:
  rC Clang

https://reviews.llvm.org/D48661