[PATCH] D10018: C11 _Bool bitfield diagnostic

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4320-4325
@@ -4319,2 +4319,8 @@
   "number of elements must be either one or match the size of the vector">;
+def warn_bitfield_width_longer_than_necessary : Warning<
+  "size of bit-field %0 (%1 bits) exceeds the minimum width required to "
+  "represent all valid values of that bit-field type">, InGroup<BitFieldWidth>;
+def warn_anon_bitfield_width_longer_than_necessary : Warning<
+  "size of anonymous bit-field (%0 bits) exceeds the minimum width required to "
+  "represent all valid values of that bit-field type">, InGroup<BitFieldWidth>;
 
----------------
I suppose your argument for the width of `_Bool` being possibly greater than 1 is that, while `_Bool` is required to be large enough to store the values 0 and 1 (6.2.5/2), it may theoretically have value bits representing 2, 4, 8, ... (even though 6.3.1.2/1 does not allow creation of a value that sets those bits). That interpretation seems consistent with C's rules.

However, for Clang, the width of `_Bool` is 1, so this is a constraint violation.

================
Comment at: lib/Sema/SemaDecl.cpp:12586
@@ -12585,3 +12585,3 @@
   if (!FieldTy->isDependentType()) {
     uint64_t TypeSize = Context.getTypeSize(FieldTy);
     if (Value.getZExtValue() > TypeSize) {
----------------
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).

================
Comment at: lib/Sema/SemaDecl.cpp:12611
@@ +12610,3 @@
+    // different platforms
+    if ((getLangOpts().C11 || getLangOpts().C99) && 
+        FieldTy->isBooleanType() &&
----------------
This is redundant: `C99` is always set whenever `C11` is (`C99` means "C99 or later"). Just test for `C99` here. I also think we should apply this rule to C89, where we support `_Bool` as an extension and use the C99 rules for it. The above check for !CPlusPlus seems right.

================
Comment at: lib/Sema/SemaDecl.cpp:12613
@@ +12612,3 @@
+        FieldTy->isBooleanType() &&
+        Value.getZExtValue() > 1) {
+      if (FieldName)
----------------
This will assert if the specified bitfield width doesn't fit in 64 bits. You can use `Value.ugt(1)` instead.

The check above for `TypeSize` has the same bug. This testcase causes an assertion to fire:

  struct X { int n : 0xffffffff * (__int128)0xffffffff * 0xffffffff; };


http://reviews.llvm.org/D10018