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

[Prasad Ghangal <prasad dot ghangal at gmail dot com>]

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