[PATCH] Fix Bug 17896: The expression (a>0 & b>0) should give clearer warning message (-Wparentheses)

[PATCH] Fix Bug 17896: The expression (a>0 & b>0) should give clearer warning message (-Wparentheses)

[Prasad Ghangal]

[-- Attachment #1: Type: text/plain, Size: 328 bytes --]

Hi!

This is my first proposed patch for
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17896. I was willing to
do it using "APPEARS_TO_BE_BOOLEAN_EXPR_P(CODE, ARG)" to check
booleans but gcc doesn't allow (bootstraping fails). Hence I am using
"TREE_CODE_CLASS (CODE) == tcc_comparison"



-- 
Thanks and Regards,
Prasad Ghangal

[-- Attachment #2: bug17896.patch --]
[-- Type: text/x-patch, Size: 1462 bytes --]

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 232768)
+++ gcc/c-family/c-common.c	(working copy)
@@ -11596,6 +11596,11 @@
 	       || code_right == PLUS_EXPR || code_right == MINUS_EXPR)
 	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
 		 "suggest parentheses around arithmetic in operand of %<|%>");
+      /* Check cases like (x<y) | (x<z)*/
+      else if(TREE_CODE_CLASS (code_left) == tcc_comparison
+	       && (TREE_CODE_CLASS (code_right) == tcc_comparison))
+	warning_at (loc, OPT_Wparentheses,
+		 "suggest %<||%> instead of %<|%> when joining booleans");
       /* Check cases like x|y==z */
       else if (TREE_CODE_CLASS (code_left) == tcc_comparison)
 	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
@@ -11642,6 +11647,11 @@
       else if (code_right == MINUS_EXPR)
 	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
 		 "suggest parentheses around %<-%> in operand of %<&%>");
+      /* Check cases like (x<y) & (x<z)*/
+      else if(TREE_CODE_CLASS (code_left) == tcc_comparison 
+	      && TREE_CODE_CLASS (code_right) == tcc_comparison)
+	warning_at (loc, OPT_Wparentheses,
+		 "suggest %<&&%> instead of %<&%> when joining booleans");
       /* Check cases like x&y==z */
       else if (TREE_CODE_CLASS (code_left) == tcc_comparison)
 	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,

Re: [PATCH] Fix Bug 17896: The expression (a>0 & b>0) should give clearer warning message (-Wparentheses)

[Prasad Ghangal]

PING
Sorry I didn't include [Fix PR c/17896] in prev mail

https://gcc.gnu.org/ml/gcc-patches/2016-01/msg02361.html

On 30 January 2016 at 19:05, Prasad Ghangal <prasad.ghangal@gmail.com> wrote:
> Hi!
>
> This is my first proposed patch for
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17896. I was willing to
> do it using "APPEARS_TO_BE_BOOLEAN_EXPR_P(CODE, ARG)" to check
> booleans but gcc doesn't allow (bootstraping fails). Hence I am using
> "TREE_CODE_CLASS (CODE) == tcc_comparison"
>
>
>
> --
> Thanks and Regards,
> Prasad Ghangal



-- 
Thanks and Regards,
Prasad Ghangal

Re: [PATCH] Fix Bug 17896: The expression (a>0 & b>0) should give clearer warning message (-Wparentheses)

[David Malcolm]

On Sat, 2016-01-30 at 19:05 +0530, Prasad Ghangal wrote:
> Hi!
> 
> This is my first proposed patch for
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17896. I was willing to
> do it using "APPEARS_TO_BE_BOOLEAN_EXPR_P(CODE, ARG)" to check
> booleans but gcc doesn't allow (bootstraping fails). Hence I am using
> "TREE_CODE_CLASS (CODE) == tcc_comparison"

Thanks for submitting this patch, and welcome!

I'm not a patch reviewer, but hopefully the following will be helpful.

Is this your first patch for GCC?  For non-trivial patches, there's
some legal rubric that must be followed; see:
https://gcc.gnu.org/contribute.html#legal

When submitting a patch it's a good idea to be explicit about what
testing you've done on it.  Typically you should do a full bootstrap
with the patch, and then run the test suite, and verify that nothing
has regressed.  So typically with a patch sent to this list you should
state that you successfully performed a bootstrap and regression
testing with the patch, and you should state which kind of host you did
this on (e.g. "x86_64-pc-linux-gnu").

See https://gcc.gnu.org/contribute.html for more information.

For a patch like this that improves a warning, it ought to include test
cases (or extend existing ones).  Some information on creating test
cases can be seen here:
https://gcc.gnu.org/wiki/HowToPrepareATestcase

There are a couple of trivial stylistic issues in the patch itself:

> Index: gcc/c-family/c-common.c
> ===================================================================
> --- gcc/c-family/c-common.c	(revision 232768)
> +++ gcc/c-family/c-common.c	(working copy)
> @@ -11596,6 +11596,11 @@
>  	       || code_right == PLUS_EXPR || code_right == MINUS_EXPR)
>  	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
> 		 "suggest parentheses around arithmetic in operand of %<|%>");
> +      /* Check cases like (x<y) | (x<z)*/
> +      else if(TREE_CODE_CLASS (code_left) == tcc_comparison

There should be a space between the "if" and the open paren here.

> +	       && (TREE_CODE_CLASS (code_right) == tcc_comparison))
> +	warning_at (loc, OPT_Wparentheses,
> +		 "suggest %<||%> instead of %<|%> when joining booleans");
>        /* Check cases like x|y==z */
>        else if (TREE_CODE_CLASS (code_left) == tcc_comparison)
>  	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
> @@ -11642,6 +11647,11 @@
>       else if (code_right == MINUS_EXPR)
>  	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
> 		 "suggest parentheses around %<-%> in operand of %<&%>");
> +      /* Check cases like (x<y) & (x<z)*/
> +      else if(TREE_CODE_CLASS (code_left) == tcc_comparison 
Likewise here.

> +	      && TREE_CODE_CLASS (code_right) == tcc_comparison)
> +	warning_at (loc, OPT_Wparentheses,
> +		 "suggest %<&&%> instead of %<&%> when joining booleans");
>       /* Check cases like x&y==z */
>       else if (TREE_CODE_CLASS (code_left) == tcc_comparison)
>  	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,

...but IMHO the main thing is to add test cases, so that we can see
what the effect of the patch is, and so that the test suite
automatically
ensures that gcc continues to do the right thing.

A full patch should also include a ChangeLog entry summarizing the
change.

One other thing to note is that currently we're in "stage 4" of
development for gcc 6, meaning that we're in deep feature freeze, and
only fixing the most severe bugs.  Given that, your patch is probably
more appropriate for gcc 7 than gcc 6.

Hope this is helpful; thanks and welcome again.
Dave

Re: [PATCH] Fix Bug 17896: The expression (a>0 & b>0) should give clearer warning message (-Wparentheses)

[Prasad Ghangal]

Thank you David for your response. I will keep all that in mind from next patch.

On 21 February 2016 at 03:37, David Malcolm <dmalcolm@redhat.com> wrote:
> On Sat, 2016-01-30 at 19:05 +0530, Prasad Ghangal wrote:
>> Hi!
>>
>> This is my first proposed patch for
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17896. I was willing to
>> do it using "APPEARS_TO_BE_BOOLEAN_EXPR_P(CODE, ARG)" to check
>> booleans but gcc doesn't allow (bootstraping fails). Hence I am using
>> "TREE_CODE_CLASS (CODE) == tcc_comparison"
>
> Thanks for submitting this patch, and welcome!
>
> I'm not a patch reviewer, but hopefully the following will be helpful.
>
> Is this your first patch for GCC?  For non-trivial patches, there's
> some legal rubric that must be followed; see:
> https://gcc.gnu.org/contribute.html#legal
>
> When submitting a patch it's a good idea to be explicit about what
> testing you've done on it.  Typically you should do a full bootstrap
> with the patch, and then run the test suite, and verify that nothing
> has regressed.  So typically with a patch sent to this list you should
> state that you successfully performed a bootstrap and regression
> testing with the patch, and you should state which kind of host you did
> this on (e.g. "x86_64-pc-linux-gnu").
>
> See https://gcc.gnu.org/contribute.html for more information.
>
> For a patch like this that improves a warning, it ought to include test
> cases (or extend existing ones).  Some information on creating test
> cases can be seen here:
> https://gcc.gnu.org/wiki/HowToPrepareATestcase
>
> There are a couple of trivial stylistic issues in the patch itself:
>
>> Index: gcc/c-family/c-common.c
>> ===================================================================
>> --- gcc/c-family/c-common.c   (revision 232768)
>> +++ gcc/c-family/c-common.c   (working copy)
>> @@ -11596,6 +11596,11 @@
>>              || code_right == PLUS_EXPR || code_right == MINUS_EXPR)
>>       warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
>>                "suggest parentheses around arithmetic in operand of %<|%>");
>> +      /* Check cases like (x<y) | (x<z)*/
>> +      else if(TREE_CODE_CLASS (code_left) == tcc_comparison
>
> There should be a space between the "if" and the open paren here.
>
>> +            && (TREE_CODE_CLASS (code_right) == tcc_comparison))
>> +     warning_at (loc, OPT_Wparentheses,
>> +              "suggest %<||%> instead of %<|%> when joining booleans");
>>        /* Check cases like x|y==z */
>>        else if (TREE_CODE_CLASS (code_left) == tcc_comparison)
>>       warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
>> @@ -11642,6 +11647,11 @@
>>       else if (code_right == MINUS_EXPR)
>>       warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
>>                "suggest parentheses around %<-%> in operand of %<&%>");
>> +      /* Check cases like (x<y) & (x<z)*/
>> +      else if(TREE_CODE_CLASS (code_left) == tcc_comparison
> Likewise here.
>
>> +           && TREE_CODE_CLASS (code_right) == tcc_comparison)
>> +     warning_at (loc, OPT_Wparentheses,
>> +              "suggest %<&&%> instead of %<&%> when joining booleans");
>>       /* Check cases like x&y==z */
>>       else if (TREE_CODE_CLASS (code_left) == tcc_comparison)
>>       warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
>
> ...but IMHO the main thing is to add test cases, so that we can see
> what the effect of the patch is, and so that the test suite
> automatically
> ensures that gcc continues to do the right thing.
>
> A full patch should also include a ChangeLog entry summarizing the
> change.
>
> One other thing to note is that currently we're in "stage 4" of
> development for gcc 6, meaning that we're in deep feature freeze, and
> only fixing the most severe bugs.  Given that, your patch is probably
> more appropriate for gcc 7 than gcc 6.
>
> Hope this is helpful; thanks and welcome again.
> Dave



-- 
Thanks and Regards,
Prasad Ghangal

Re: [PATCH] Fix Bug 17896: The expression (a>0 & b>0) should give clearer warning message (-Wparentheses)

[Jeff Law]

On 01/30/2016 06:35 AM, Prasad Ghangal wrote:
> Hi!
>
> This is my first proposed patch for
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17896. I was willing to
> do it using "APPEARS_TO_BE_BOOLEAN_EXPR_P(CODE, ARG)" to check
> booleans but gcc doesn't allow (bootstraping fails). Hence I am using
> "TREE_CODE_CLASS (CODE) == tcc_comparison"
I would encourage you to look more closely at why GCC failed to 
bootstrap.  It could be the case that APPEARS_TO_BE_BOOLEAN_EXPR_P is 
the wrong test to use, or it could be a bug in GCC itself.

>
>
>
> -- Thanks and Regards, Prasad Ghangal
>
>
> bug17896.patch
>
>
> Index: gcc/c-family/c-common.c
> ===================================================================
> --- gcc/c-family/c-common.c	(revision 232768)
> +++ gcc/c-family/c-common.c	(working copy)
> @@ -11596,6 +11596,11 @@
>  	       || code_right == PLUS_EXPR || code_right == MINUS_EXPR)
>  	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
>  		 "suggest parentheses around arithmetic in operand of %<|%>");
> +      /* Check cases like (x<y) | (x<z)*/
Space before the close comment.

> +      else if(TREE_CODE_CLASS (code_left) == tcc_comparison
> +	       && (TREE_CODE_CLASS (code_right) == tcc_comparison))
> +	warning_at (loc, OPT_Wparentheses,
> +		 "suggest %<||%> instead of %<|%> when joining booleans");
Space between the "if" and open paren.

Both of those nits are repeated in the second block of code.

I note in the patch inside the BZ that the original author verified 
neither argument had side effects.  Any reason you dropped that test?

Similarly in that patch there's a test for the testsuite.  AFAICT your 
patch doesn't even pass that test.  For a change like yours we 
definitely want to include a reasonable test -- you could start with the 
one included in the BZ or construct your own.

As of right now I don't think your patch needs a copyright assignment, 
but that might change if the patch grows too much.


Jeff