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
