[PATCH] D63082: [Diagnostics] Added support for -Wint-in-bool-context

Tue Jul 2 14:03:40 PDT 2019

xbolva00 added a comment.

In D63082#1564114 <https://reviews.llvm.org/D63082#1564114>, @aaron.ballman wrote:

> In D63082#1560667 <https://reviews.llvm.org/D63082#1560667>, @xbolva00 wrote:
>
> > I wanted to follow GCC’s behaviour -Wall but then dicussion moved to “tautological compare group” - I was fine with that too..
> >  I can again revert it to -Wall...
> >  If this should be off by default even on -Wall, then all work was useless..
>
>
> Personally, my feeling is that this new diagnostic group belongs under the tautological compare group. That group is currently off by default, but I'm wondering if we want it to be on by default (in a subsequent patch), or whether we're okay having parts of it on by default and parts off by default. I'm hoping @rsmith will voice an opinion here.
>
> In D63082#1560684 <https://reviews.llvm.org/D63082#1560684>, @xbolva00 wrote:
>
> > “Perhaps appending something about the resulting value always being true|false would help most of these diagnostics be more obvious”
> >
> > I dont know, this could only diagnose constant multiplications (maybe this is already handled by some “always true” check). Anyway, I have no motivation to diagnose constant muls, sorry. My goal is to make this warning atleast as good as GCC’s implementation.
> >
> > Okay, we could make it better and append “; did you mean “index * 3 != 0”?
>
>
> I don't think that actually improves anything. I do not think we want a "did you mean" in this case because I'm not confident anyone can guess what a user was trying for in this situation. However, telling the user the result of the computation does give them very useful information -- it tells them the result will always be tautologically true or false (which is what the other tautological warnings do, as well). As it stands, the current diagnostic wording doesn't help the user understand what's wrong with their code. I'd like to correct that deficiency before we commit this.

Thanks

I decided to implement your suggestions.

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

https://reviews.llvm.org/D63082