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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/Basic/TargetInfo.h:90
+  // by default.
+  bool SameFBits;
+
----------------
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.


================
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>;
 
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D46915