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: > aaron.ballman wrote: > > ShawnZhong wrote: > > > 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 > > > Do you have something in mind that counts as false positives? > > > > In my example above, I consider the use of `s.bit` and `!s.bit` to count as > > false positives. The value in the bit-field being 1 or -1 has no bearing on > > the semantics of the program, the only thing that matters is zero vs > > nonzero because of the boolean conversion. > > > > > BTW, I realized that no warning is reported when a bool is assigned to a > > > single-bit signed bit-field: > > > > Good catch, the conversion from bool to integer does give the value `1` > > explicitly. At the same time, I would consider the assignment to the > > bit-field from a boolean to be a non-issue, the problem only stems from > > attempting to use inspect the value. > >>BTW, I realized that no warning is reported when a bool is assigned to a > >>single-bit signed bit-field: > > Good catch, the conversion from bool to integer does give the value 1 > > explicitly. At the same time, I would consider the assignment to the > > bit-field from a boolean to be a non-issue, the problem only stems from > > attempting to use inspect the value. > > Actually, that's a more interesting case than I had originally realized. See > C2x 6.7.2.1p12. "If the value 0 or 1 is stored into a nonzero-width bit-field > of type bool, the value of the bit-field shall compare equal to the value > stored; a bool bit-field has the semantics of a bool." > > So if you store a 1 into a bool bit-field, you have to get a 1 back out of > it. The bit-field is implicitly unsigned, effectively. > In my example above, I consider the use of `s.bit` and `!s.bit` to count as > false positives. The value in the bit-field being 1 or -1 has no bearing on > the semantics of the program, the only thing that matters is zero vs nonzero > because of the boolean conversion. I agree with you that if `s.bit` is only used to be converted to `bool`, then the value stored being 1 or -1 does not matter that much. But hypothetically, one could use `s.bit` for arithmetic (e.g., index into an array), assignment (say into another int or enum), or, in your example, comparison against another value. If a warning is not generated during conversion, handling them in other places would be harder. > Actually, that's a more interesting case than I had originally realized. See > C2x 6.7.2.1p12. "If the value 0 or 1 is stored into a nonzero-width bit-field > of type bool, the value of the bit-field shall compare equal to the value > stored; a bool bit-field has the semantics of a bool." > So if you store a 1 into a bool bit-field, you have to get a 1 back out of > it. The bit-field is implicitly unsigned, effectively. That makes sense to me. I'm not particularly concerned with assigning 1 to `bool:1` or any `uint*_t:1` in general. People may want to use them to compactly store `bool`s in a struct. > I realized that no warning is reported when bool is assigned to a single-bit > signed bit-field The issue I referred to is the following: ``` int main() { struct S { int b : 1; } s; s.b = 1; // warning generated after this patch s.b = true; // still no warning, but there should be } ``` 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
