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; ---------------- ShawnZhong wrote: > 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"); } > } > ``` > > For reference, I found this issue on chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1352183 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
