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