On 08/25/2016 12:02 PM, David Malcolm wrote:
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.
I suppose it is. I conflated the two patches: the one to Add fixit hint for -Wlogical-not-parentheses and this one. I didn't mean to hijack this review with the comment. Maybe I should follow up in the other thread... Sorry!
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).
Yes, exactly. In my ignorance that's what I thought the fixit hints were doing. Under the control of the caller rewriting the expression the suggested way. Now that I've looked more closely at the first of the two patches I see that the hint is inserted as a plain old string. Martin
