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