[PATCH] D132851: Further update -Wbitfield-constant-conversion for 1-bit bitfield

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/test/Sema/constant-conversion.c:30
+  s.b = 1;     // one-bit-warning {{implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1}}
+  s.b = true;  // no-warning (we suppress it manually to reduce false positives)
+  s.b = false; // no-warning
----------------
bjope wrote:
> (Sorry for being late to the party, with post commit comments. I'm just trying to understand the reasoning about always suppressing the warning based on "true".)
> 
> Isn't the code a bit suspicious when using true/false from stdbool.h, while using a signed bitfield?
> 
> When doing things like 
> ```
> (s.b == true) ? 1 : 0
> ```
> the result would always be zero if s.b is a signed 1-bit bitfield.
> 
> So wouldn't it make more sense to actually use an unsigned bitfield (such as the bool type from stdbool.h) when the intent is to store a boolean value and using defines from stdbool.h?
> 
> Is perhaps the idea that we will get warnings about `(s.b == true)` being redundant in situations like this, and then we do not need a warning on the bitfield assignment? Such a reasoning would actually make some sense, since `(s.b == true)` never would be true even when the bitfield is assigned a non-constant value, so we can't rely on catching the problem by only looking at bitfield assignments involving true/false.
> Isn't the code a bit suspicious when using true/false from stdbool.h, while using a signed bitfield?

Yes and no...

> When doing things like
> ```
> (s.b == true) ? 1 : 0
> ```
> the result would always be zero if s.b is a signed 1-bit bitfield.

You're correct, but that's a bit of a code smell because of `== true`.

> So wouldn't it make more sense to actually use an unsigned bitfield (such as the bool type from stdbool.h) when the intent is to store a boolean value and using defines from stdbool.h?

Yes, ideally.

> Is perhaps the idea that we will get warnings about (s.b == true) being redundant in situations like this, and then we do not need a warning on the bitfield assignment? Such a reasoning would actually make some sense, since (s.b == true) never would be true even when the bitfield is assigned a non-constant value, so we can't rely on catching the problem by only looking at bitfield assignments involving true/false.

The idea is more that someone using `true` is more likely to be treating the field as a boolean and so they won't be comparing the bit-field against a specific value, but instead testing it for its boolean value. This also unifies the behavior between C and C++:  https://godbolt.org/z/WKP4xcPzT

We don't currently issue a diagnostic on `== true`, but that sure seems like something `-Wtautological-compare` should pick up on (for bit-fields in general, not just for one-bit bit-fields): https://godbolt.org/z/Tj4711Ysc

(Note, godbolt seems to have a Clang trunk that's ~8 days old, so diagnostic behavior doesn't match actual trunk yet. That caught me by surprise.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132851