[PATCH] D127287: clang: Introduce -fexperimental-max-bitint-width

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Given that this is a hidden option we expect to remove... do we add a release note for it? I think we probably should (for user awareness in case more users are in the same situation as the original reporter), and that gives a place to document that we will be removing the option in the future.



================
Comment at: clang/include/clang/Basic/LangOptions.def:448
 
+BENIGN_VALUE_LANGOPT(MaxBitIntWidth, 32, 128, "Maximum width of a _BitInt")
+
----------------
Should we add a comment about how this is expected to be removed in the future once the backend work is fixed?


================
Comment at: clang/include/clang/Basic/TargetInfo.h:239
 
+  unsigned MaxBitIntWidth;
+
----------------
Do we want to make this into an `Optional<unsigned>` so we don't have to use 0 as a sentinel value? (I don't feel strongly, but we already use `Optional` in this interface anyways.)


================
Comment at: clang/include/clang/Driver/Options.td:6088
+  MetaVarName<"<N>">,
+  HelpText<"Set the maximum bitwidth for _BitInt (experimental)">,
+  MarshallingInfoInt<LangOpts<"MaxBitIntWidth">, "0">;
----------------
Maybe?


================
Comment at: clang/lib/Serialization/ASTReader.cpp:317
 #define BENIGN_ENUM_LANGOPT(Name, Type, Bits, Default, Description)
-#define BENIGN_VALUE_LANGOPT(Name, Type, Bits, Default, Description)
+#define BENIGN_VALUE_LANGOPT(Name, Bits, Default, Description)
 #include "clang/Basic/LangOptions.def"
----------------
Good catch!


================
Comment at: clang/test/Sema/large-bit-int.c:1
+// RUN: %clang_cc1 -fexperimental-max-bitint-width=1024 -fsyntax-only -verify %s -Wno-unused -triple x86_64-gnu-linux
+
----------------
Thoughts on adding a frontend diagnostic if the user specifies a value larger than `llvm::IntegerType::MAX_INT_BITS`? I'm on the fence. OTOneH, this gives a good user experience in the event of mistypes in the command line, OTOtherH, it's a cc1-only option that we expect to remove in the (hopefully) near future.

Also, I don't think the triple is needed and I'm pretty sure `-Wno-unused` is the default already.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127287/new/

https://reviews.llvm.org/D127287