[PATCH] D63856: [ObjC] Add a -Wtautological-compare warning for BOOL

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In D63856#1561112 <https://reviews.llvm.org/D63856#1561112>, @rjmccall wrote:

> In D63856#1560213 <https://reviews.llvm.org/D63856#1560213>, @erik.pilkington wrote:
>
> > In D63856#1560180 <https://reviews.llvm.org/D63856#1560180>, @rjmccall wrote:
> >
> > > This only applies to relational operators, right?  I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have `(BOOL) 2`, it's just *unwise*.  But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-`{0,1}` value in the `BOOL` — I think this is fine.
> >
> >
> > I think the party line is that it is undefined behaviour (in some sense), since UBSan will happily crash if you try to load a non-boolean value from a BOOL.
>
>
> What?  Since when?


https://reviews.llvm.org/D27607

> 
> 
>> It is a bit unfortunate that "defensive" code will start warning here though :/. Maybe we can try to detect and permit something like `B < NO || B > YES`, or emit a note with some canonical way of checking for non-boolean BOOLs. Even if we end up having to disable it default, I think its still a good diagnostic to have. A warning on stores to BOOL would probably be a lot higher value, though.
> 
> I'm not sure this is a problem because I'm not sure there's any reason to write defensive code besides `B != NO` or `B == NO`.  It's potentially problematic if someone writes `B == YES`, though.

I was thinking about something like the following, which probably isn't worth warning on. Another way of handling it would be only emitting the diagnostic if `!InRange`. Not exactly sure how common that pattern actually is though.

  void myAPI(BOOL DoAThing) {
    if (DoAThing > YES || DoAThing < NO) DoAThing = YES;
    // ...
  }


Repository:
  rC Clang

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

https://reviews.llvm.org/D63856