Re: c-family PATCH to improve -Wtautological-compare (PR c/81783)

[Marek Polacek <polacek at redhat dot com>]

On Wed, Aug 16, 2017 at 11:07:36AM -0400, David Malcolm wrote:
> On Wed, 2017-08-16 at 16:29 +0200, Marek Polacek wrote:
> > This patch improves -Wtautological-compare so that it also detects
> > bitwise comparisons involving & and | that are always true or false,
> > e.g.
> > 
> >   if ((a & 16) == 10)
> >     return 1;
> > 
> > can never be true.  Note that e.g. "(a & 9) == 8" is *not* always
> > false
> > or true.
> > 
> > I think it's pretty straightforward with one snag: we shouldn't warn
> > if
> > the constant part of the bitwise operation comes from a macro, but
> > currently
> > that's not possible, so I XFAILed this in the new test.
> 
> Maybe I'm missing something here, but why shouldn't it warn when the
> constant comes from a macro?

Just my past experience.  Sometimes you can't really control the macro
and then you get annoying warnings.

E.g. I had to tweak the warning that warns about if (i == i) to not warn about
  
  #define N 2
  if (a[N] == a[2]) {}

because that gave bogus warning during bootstrap, if I recall well.

> At the end of your testcase you have this example:
> 
> #define N 0x10
>   if ((a & N) == 10) /* { dg-bogus "bitwise comparison always evaluates to false" "" { xfail *-*-* } } */
>      return 1;
>   if ((a | N) == 10) /* { dg-bogus "bitwise comparison always evaluates to false" "" { xfail *-*-* } } */
>    return 1;
> 
> That code looks bogus to me (and if the defn of "N" is further away,
> it's harder to spot that it's wrong): shouldn't we warn about it?

I'm glad you think so.  More than happy to make it an expected warning.

> > This has found one issue in the GCC codebase and it's a genuine bug:
> > <https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00757.html>.  
> 
> In this example GOVD_WRITTEN is from an enum, not a macro, but if
> GOVD_WRITTEN had been a macro, shouldn't we still issue a warning?

I feel like we should, but some might feel otherwise.

Thanks,

	Marek