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