[PATCH] D131255: Fix Wbitfield-constant-conversion on 1-bit signed bitfield

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13069
-  // therefore don't strictly fit into a signed bitfield of width 1.
-  if (FieldWidth == 1 && Value == 1)
-    return false;
----------------
thakis wrote:
> This was to suppress false positives. All instances we've seen of this are in fact false positives.
> 
> Was there any analysis on true / false positives for this change?
> 
> At least for our code, this seems to make the warning strictly worse.
I've not performed any such analysis, but it would be good to know. FWIW, this is the kind of situation I was thinking this diagnostic would help make far more clear: https://godbolt.org/z/336n9xex3 (not that I expect people to write that static assert, but I very much expect people who assign the value 1 into a bit-field and then check for the value 1 to come back out are going to be surprised).

That said, another approach to that specific scenario, which might have a better true positive rate would be:
```
struct S {
  int bit : 1;
};

int main() {
  constexpr S s{1}; // No warning
  if (s.bit == 1) { // Warn about explicit comparison of a 1-bit bit-field
    ...
  } else if (s.bit == 0) { // Warn about explicit comparison of a 1-bit bit-field?
    ...
  } else if (s.bit) { // No warning
    ...
  } else if (!s.bit) { // No warning
    ...
  }
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131255