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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith 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();
----------------
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.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:16448
+      
+      if (BitsNeeded > Value.getZExtValue()) {
+        // TODO: Should we be emitting diagnostics for unnamed bitfields that
----------------
> 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.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:16454-16459
+        } else {
+          Diag(FieldLoc, diag::warn_bitfield_too_small_for_enum)
+              << "<unnamed>" << ED;
+              //<< FieldName << ED;
+              //
+        }
----------------
> Should we be emitting such a warning for unnamed bitfields?

The warning is not useful for unnamed bit-fields, as they represent only padding and not values. We should suppress it in that case.

(Maybe we should have a warning for unnamed bit-fields of "unusual" types, since that probably indicates that a name was forgotten, but there might also be valid reasons to do that, such as to avoid the problematic corners of the MS ABI bit-field layout rule. So it seems better to suppress it entirely.)


================
Comment at: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp:62
+
+#ifdef FV_UNNAMED_BITFIELD
+#define NAME(x)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91651