[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: include/clang/Basic/TargetInfo.h:90
+  // by default.
+  bool SameFBits;
+
----------------
rjmccall wrote:
> Sorry for the extremely late review, but this really needs to be renamed.  Please remember that other compiler maintainers are not experts in the feature that you happen to be currently implementing; they should be able to look at a variable name and understand what it means, and honestly "fbits" does not mean anything.  `UnsignedFixedPointHasNoPadding`, maybe?  Or you could shorten it by switching the polarity.
Made a new diff: https://reviews.llvm.org/D48727.

Opted for renaming to PaddingOnUnsignedFixedPoint.


================
Comment at: include/clang/Driver/Options.td:899
+		  HelpText<"Force each unsigned fixed point type to have the same number of fractional bits as its corresponding signed type">;
+def fno_same_fbits : Flag<["-"], "fno-same-fbits">, Group<f_Group>;
 
----------------
rjmccall wrote:
> Okay, please also remember that *users* are not deeply embedded into the lore and wisdom of your implementation group. :)  This is not an acceptable user-facing option name; please rename it to something with "fixed-point" in the name.
> 
> Also, if this is just for testing (it seems a bit early to decide that users will need an ability to override their target about this?), maybe it should just be a -cc1 option.
Renamed to `-fpadding-on-unsigned-fixed-point` in https://reviews.llvm.org/D48727


Repository:
  rC Clang

https://reviews.llvm.org/D46915