Re: Fix bogus warning with -Wlogical-not-parentheses (PR c/77292)

[David Malcolm <dmalcolm at redhat dot com>]

On Thu, 2016-08-25 at 11:35 -0600, Martin Sebor wrote:
> > +foo (int a, int b)
> > +{
> > +  int r = 0;
> > +  r += !a == (a < b);
> > +  r += !a == (a > b);
> > +  r += !a == (a >= b);
> > +  r += !a == (a <= b);
> > +  r += !a == (a != b);
> > +  r += !a == (a == b);
> > +  r += !a == (a || b);
> > +  r += !a == (a && b);
> > +  r += !a == (!b);
> > +
> > +  r += !a == (a ^ b); /* { dg-warning "logical not is only applied
> > to the left hand side of comparison" } */
> > +  r += !a == (a | b); /* { dg-warning "logical not is only applied
> > to the left hand side of comparison" } */
> > +  r += !a == (a & b); /* { dg-warning "logical not is only applied
> > to the left hand side of comparison" } */
> 
> A question more than a comment: warning on the last three expressions
> above makes sense to me when the operands are integers but I'm less
> sure that the same logic applies when the operands are Boolean.  Does
> it make sense to issue the warning for those given that (a | b) and
> (a & b) are the same as (a || b) and (a && b) for which the warning
> is suppressed?
> 
> In other words, is warning on the latter of the two below but not on
> the former a good idea or should they be treated the same way?
> 
> $ cat t.c && gcc -S -Wlogical-not-parentheses t.c
> _Bool f (int a, _Bool b, _Bool c)
> {
>    _Bool r;
> 
>    r = !a == (b || c);
>    r = !a == (b | c);
> 
>    return r;
> }
> t.c: In function ‘f’:
> t.c:6:10: warning: logical not is only applied to the left hand side
> of 
> comparison [-Wlogical-not-parentheses]
>     r = !a == (b | c);
>            ^~
> t.c:6:7: note: add parentheses around left hand side expression to 
> silence this warning
>     r = !a == (b | c);
>         ^~
>         ( )
> 
> Also, having hardly any experience with the fixit hints, I'm not
> sure how helpful this one is.  It seems really hard to tell what
> it suggests as the fix (I had to try it both ways to see).  I
> think would be a lot clearer if it showed the full expression
> rather than just the operators.  Would printing this instead be
> doable?
> 
>     r = !a == (b | c);
>         ^~
>         (!a)

This seems to me to be more about how we print fix-it hints in general
that about the specific usage of them by this diagnostic.

It would be possible to rewrite the fix-it printing routine in
diagnostic-show-locus.c to print the affected parts of the line
(perhaps using the edit_context class from my recent patch kit).