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

[David Malcolm <dmalcolm at redhat dot com>]

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