| Message ID | 20160825133945.GQ11131@redhat.com |
|---|---|
| State | New |
| Headers | show |
> +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) Martin
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).
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
On Thu, Aug 25, 2016 at 3:39 PM, Marek Polacek <polacek@redhat.com> wrote: > On Thu, Aug 25, 2016 at 11:17:37AM +0200, Richard Biener wrote: >> On Wed, Aug 24, 2016 at 7:43 PM, Marek Polacek <polacek@redhat.com> wrote: >> > The -Wlogical-not-parentheses deliberately doesn't warn when the RHS has >> > boolean type. But since in C the type of a comparison is int, we need >> > to check for tcc_comparison, too. >> > >> > Bootstrapped/regtested on x86_64-linux and ppc64le-redhat-linux, ok for trunk? >> >> What about using truth_value_p ()? That also includes && and || (just >> in case those >> do not have boolean type). > > You're right that I should also handle || and && (done in this patch). But I > can't use truth_value_p because we want to warn for e.g. "!a == (a & b)". But a & b isn't truth_value_p. a && b is. Do we want to warn about !a == (a && b)? Richard. > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2016-08-25 Marek Polacek <polacek@redhat.com> > > PR c/77292 > * c-common.c (warn_logical_not_parentheses): Don't warn for > a comparison or a logical operator. > > * c-c++-common/Wlogical-not-parentheses-1.c: New test. > > diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c > index 3feb910..3656c57 100644 > --- gcc/c-family/c-common.c > +++ gcc/c-family/c-common.c > @@ -1481,7 +1481,8 @@ warn_tautological_cmp (location_t loc, enum tree_code code, tree lhs, tree rhs) > > /* Warn about logical not used on the left hand side operand of a comparison. > This function assumes that the LHS is inside of TRUTH_NOT_EXPR. > - Do not warn if RHS is of a boolean type. */ > + Do not warn if RHS is of a boolean type, a logical operator, or > + a comparison. */ > > void > warn_logical_not_parentheses (location_t location, enum tree_code code, > @@ -1489,7 +1490,10 @@ warn_logical_not_parentheses (location_t location, enum tree_code code, > { > if (TREE_CODE_CLASS (code) != tcc_comparison > || TREE_TYPE (rhs) == NULL_TREE > - || TREE_CODE (TREE_TYPE (rhs)) == BOOLEAN_TYPE) > + || TREE_CODE (TREE_TYPE (rhs)) == BOOLEAN_TYPE > + || COMPARISON_CLASS_P (rhs) > + || TREE_CODE (rhs) == TRUTH_ANDIF_EXPR > + || TREE_CODE (rhs) == TRUTH_ORIF_EXPR) > return; > > /* Don't warn for !x == 0 or !y != 0, those are equivalent to > diff --git gcc/testsuite/c-c++-common/Wlogical-not-parentheses-1.c gcc/testsuite/c-c++-common/Wlogical-not-parentheses-1.c > index e69de29..ba57437 100644 > --- gcc/testsuite/c-c++-common/Wlogical-not-parentheses-1.c > +++ gcc/testsuite/c-c++-common/Wlogical-not-parentheses-1.c > @@ -0,0 +1,25 @@ > +/* PR c/77292 */ > +/* { dg-do compile } */ > +/* { dg-options "-Wlogical-not-parentheses" } */ > + > + /* Test that we don't warn if rhs is a comparison or a logical op. */ > + > +int > +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" } */ > + return r; > +} > > Marek
On Fri, Aug 26, 2016 at 10:53:01AM +0200, Richard Biener wrote: > On Thu, Aug 25, 2016 at 3:39 PM, Marek Polacek <polacek@redhat.com> wrote: > > On Thu, Aug 25, 2016 at 11:17:37AM +0200, Richard Biener wrote: > >> On Wed, Aug 24, 2016 at 7:43 PM, Marek Polacek <polacek@redhat.com> wrote: > >> > The -Wlogical-not-parentheses deliberately doesn't warn when the RHS has > >> > boolean type. But since in C the type of a comparison is int, we need > >> > to check for tcc_comparison, too. > >> > > >> > Bootstrapped/regtested on x86_64-linux and ppc64le-redhat-linux, ok for trunk? > >> > >> What about using truth_value_p ()? That also includes && and || (just > >> in case those > >> do not have boolean type). > > > > You're right that I should also handle || and && (done in this patch). But I > > can't use truth_value_p because we want to warn for e.g. "!a == (a & b)". > > But a & b isn't truth_value_p. a && b is. Do we want to warn about > !a == (a && b)? We don't want to warn about !a == (a && b), I think. But truth_value_p returns true for both TRUTH_AND_EXPR and TRUTH_ANDIF_EXPR. I find that confusing, and I can't use it in the current form because of that. Marek
On Fri, Aug 26, 2016 at 10:57 AM, Marek Polacek <polacek@redhat.com> wrote: > On Fri, Aug 26, 2016 at 10:53:01AM +0200, Richard Biener wrote: >> On Thu, Aug 25, 2016 at 3:39 PM, Marek Polacek <polacek@redhat.com> wrote: >> > On Thu, Aug 25, 2016 at 11:17:37AM +0200, Richard Biener wrote: >> >> On Wed, Aug 24, 2016 at 7:43 PM, Marek Polacek <polacek@redhat.com> wrote: >> >> > The -Wlogical-not-parentheses deliberately doesn't warn when the RHS has >> >> > boolean type. But since in C the type of a comparison is int, we need >> >> > to check for tcc_comparison, too. >> >> > >> >> > Bootstrapped/regtested on x86_64-linux and ppc64le-redhat-linux, ok for trunk? >> >> >> >> What about using truth_value_p ()? That also includes && and || (just >> >> in case those >> >> do not have boolean type). >> > >> > You're right that I should also handle || and && (done in this patch). But I >> > can't use truth_value_p because we want to warn for e.g. "!a == (a & b)". >> >> But a & b isn't truth_value_p. a && b is. Do we want to warn about >> !a == (a && b)? > > We don't want to warn about !a == (a && b), I think. But truth_value_p returns > true for both TRUTH_AND_EXPR and TRUTH_ANDIF_EXPR. I find that confusing, and > I can't use it in the current form because of that. Note that a & b would be BIT_AND_EXPR, TRUTH_AND_EXPR is just non-short-circuiting && Richard. > Marek
On Thu, Aug 25, 2016 at 11:35:53AM -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? I gave this a shot but it seems to be quite complicated, and I'm not sure if it's worth the effort. If you want, open a BZ and I'll look into this later. > 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) David answered this and I don't have a strong opinion on this. Marek
On 08/26/2016 07:44 AM, Marek Polacek wrote: > On Thu, Aug 25, 2016 at 11:35:53AM -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? > > I gave this a shot but it seems to be quite complicated, and I'm not > sure if it's worth the effort. If you want, open a BZ and I'll look > into this later. I opened bug 77423 for this. Martin
diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c index 3feb910..3656c57 100644 --- gcc/c-family/c-common.c +++ gcc/c-family/c-common.c @@ -1481,7 +1481,8 @@ warn_tautological_cmp (location_t loc, enum tree_code code, tree lhs, tree rhs) /* Warn about logical not used on the left hand side operand of a comparison. This function assumes that the LHS is inside of TRUTH_NOT_EXPR. - Do not warn if RHS is of a boolean type. */ + Do not warn if RHS is of a boolean type, a logical operator, or + a comparison. */ void warn_logical_not_parentheses (location_t location, enum tree_code code, @@ -1489,7 +1490,10 @@ warn_logical_not_parentheses (location_t location, enum tree_code code, { if (TREE_CODE_CLASS (code) != tcc_comparison || TREE_TYPE (rhs) == NULL_TREE - || TREE_CODE (TREE_TYPE (rhs)) == BOOLEAN_TYPE) + || TREE_CODE (TREE_TYPE (rhs)) == BOOLEAN_TYPE + || COMPARISON_CLASS_P (rhs) + || TREE_CODE (rhs) == TRUTH_ANDIF_EXPR + || TREE_CODE (rhs) == TRUTH_ORIF_EXPR) return; /* Don't warn for !x == 0 or !y != 0, those are equivalent to diff --git gcc/testsuite/c-c++-common/Wlogical-not-parentheses-1.c gcc/testsuite/c-c++-common/Wlogical-not-parentheses-1.c index e69de29..ba57437 100644 --- gcc/testsuite/c-c++-common/Wlogical-not-parentheses-1.c +++ gcc/testsuite/c-c++-common/Wlogical-not-parentheses-1.c @@ -0,0 +1,25 @@ +/* PR c/77292 */ +/* { dg-do compile } */ +/* { dg-options "-Wlogical-not-parentheses" } */ + + /* Test that we don't warn if rhs is a comparison or a logical op. */ + +int +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" } */ + return r; +}