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 <dmalc...@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
