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