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

[Shawn Zhong via Phabricator via cfe-commits]

ShawnZhong 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;
----------------
aaron.ballman wrote:
> 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
>     ...
>   }
> }
> ```
Do you have something in mind that counts as false positives? 


================
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;
----------------
ShawnZhong wrote:
> aaron.ballman wrote:
> > 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
> >     ...
> >   }
> > }
> > ```
> Do you have something in mind that counts as false positives? 
BTW, I realized that no warning is reported when a bool is assigned to a single-bit signed bit-field:

https://godbolt.org/z/M5ex48T8s

```
int main() {
  struct S { int b : 1; } s;
  s.b = true;
  if (s.b == true) { puts("T"); } else { puts("F"); }
}
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131255