This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix Bug 17896: The expression (a>0 & b>0) should give clearer warning message (-Wparentheses)
- From: Jeff Law <law at redhat dot com>
- To: Prasad Ghangal <prasad dot ghangal at gmail dot com>, gcc-patches at gcc dot gnu dot org
- Date: Wed, 13 Jul 2016 13:46:30 -0600
- Subject: Re: [PATCH] Fix Bug 17896: The expression (a>0 & b>0) should give clearer warning message (-Wparentheses)
- Authentication-results: sourceware.org; auth=none
- References: <CAE+uiWazXi+vnermVrp8sB710iXJ8=d5M-aufkQt2n=1UBcW8Q@mail.gmail.com>
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