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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
