[PATCH] D91651: [clang] Add a warning (à la gcc) for too small enum bitfields

[Faisal Vali via Phabricator via cfe-commits]

faisalv planned changes to this revision.
faisalv marked 3 inline comments as done.
faisalv added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:16443-16446
+      const unsigned BitsNeeded =
+          IsSignedEnum
+              ? std::max(ED->getNumPositiveBits() + 1, ED->getNumNegativeBits())
+              : ED->getNumPositiveBits();
----------------
rsmith wrote:
> Please add a member function to `EnumDecl` to compute this. SemaChecking.cpp does the same calculation twice, and CGExpr.cpp does it once too, so it seems past time to factor out this calculation.
Done.  Will try and commit a separate patch that replaces those uses, once this one has been approved.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:16448
+      
+      if (BitsNeeded > Value.getZExtValue()) {
+        // TODO: Should we be emitting diagnostics for unnamed bitfields that
----------------
rsmith wrote:
> > When comparing an APSInt to an unsigned value - should i prefer using the overloaded operator (i.e. Value < BitsNeeded) or stick with BitsNeeded > Value.getZExtValue().
> 
> Never use `get*ExtValue` unless you've already checked that the integer fits in 64 bits (and even then, it's better to avoid it when you can). For example, due to the incorrect pre-existing use of `getZExtValue` in this function, we asserted on:
> 
> ```
> enum E {};
> struct X { E e : (__int128)0x7fff'ffff'ffff'ffff * (__int128)0x7fff'ffff'ffff'ffff; };
> ```
> 
> I've fixed that in rG8e923ec2a803d54154aaa0079c1cfcf146b7a22f, but we should avoid reintroducing uses of `getZExtValue` here.
Aah!  I'm glad I asked.  Thanks!


================
Comment at: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp:62
+
+#ifdef FV_UNNAMED_BITFIELD
+#define NAME(x)
----------------
rsmith wrote:
> Please remove the `FV_` prefix here -- authorship information is in the version control history if we need it.
> 
> It would also be better to write out the test twice (once with named and once with unnamed bit-fields) instead of using a macro and running the entire test file an additional time. Each `clang` invocation adds to our total test time much more substantially than a few more lines of testcase do, and non-macro-expanded testcases are easier to understand and maintain.
> Please remove the `FV_` prefix here -- authorship information is in the version control history if we need it.
> 

Hah - that wasn't a signifier of authorship but rather employment of a well known acronym in military circles (Field Verification :  https://www.acronymfinder.com/Field-Verification-(Census)-(FV).html )  Lol - can't slip anything by you Richard ;)

> It would also be better to write out the test twice (once with named and once with unnamed bit-fields) instead of using a macro and running the entire test file an additional time. Each `clang` invocation adds to our total test time much more substantially than a few more lines of testcase do, and non-macro-expanded testcases are easier to understand and maintain.

Good to know.   Thanks. Suppression of the warning for unnamed bitfields made that test case even simpler.  



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91651