On Thu, Sep 10, 2015 at 2:17 PM, Rachel Craik <rcr...@ca.ibm.com> wrote:
> rcraik added inline comments.
>
> ================
> Comment at: lib/Sema/SemaDecl.cpp:12586
> @@ -12585,3 +12585,3 @@
> if (!FieldTy->isDependentType()) {
> uint64_t TypeSize = Context.getTypeSize(FieldTy);
> if (Value.getZExtValue() > TypeSize) {
> ----------------
> hubert.reinterpretcast wrote:
> > rsmith wrote:
> > > I think the right way to fix this is to call `getIntWidth` here
> instead of `getTypeSize`, and finesse our error message to clarify that
> we're talking about the width of the type (the number of value bits) rather
> than the size of the type (the number of storage bits).
> > The implementation of `getIntWidth` currently makes this consideration
> moot at this time, but should this extend to C89 (aside from the `_Bool`
> extension)?
> I think we have three options (the special case for _Bool bitfields being
> removed in each case):
> # change `getTypeSize` to `getIntWidth` and leave the rest of the checks
> as-is
> # change `getTypeSize` to `getIntWidth` and update the C/MS diagnostic
> to either `ExtWarn` or `Warning` (for some or all language levels)
> # leave as `getTypeSize` for lower language levels
>
> Opinions?
All of our standard modes are "language standard plus all applicable defect
reports", and the relevant constraint was added by C DR262, so it should
apply in C89 mode too, and we should pick either the first option or the
second option with an `ExtWarn`. (But this is academic, because our C89
mode does not support any integral types with padding bits other than
`_Bool`.)
I have no particular preference between the first and second (`ExtWarn`)
options. This seems like a completely reasonable extension, but we
shouldn't add an extension here unless there's actually some benefit from
doing so (some existing nontrivial codebase relying on bool bitfields with
width > 1).
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
