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:
> aaron.ballman wrote:
> > 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.)
> Ok, thanks! Sounds reasonable to me.
Great! Thank you for the discussion! (FWIW, it's *totally* fine to give post
commit feedback, so no need to apologize. You're never late to the "why does
this work that way?" party.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132851/new/
https://reviews.llvm.org/D132851
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
