| Message ID | AM4PR0701MB2162FAB66D1F87E5197975AAE4D30@AM4PR0701MB2162.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On 10/18/2016 12:14 PM, Bernd Edlinger wrote: > On 10/18/16 19:05, Joseph Myers wrote: >> > On Tue, 18 Oct 2016, Bernd Edlinger wrote: >> > >>> >> Hi, >>> >> >>> >> this restricts the -Wint-in-bool-context warning to signed shifts, >>> >> to reduce the number of false positives Markus reported yesterday. >> > >> > This patch seems to be missing testcases (that warned before the patch >> > and don't warn after it). >> > > Yes of course. > > New patch, this time with a test case, compiled from the linux sample. > > Bootstrapped and reg-tested as usual. > Is it OK for trunk? > > > Bernd. > > > patch-bool-context4.diff > > > c-family: > 2016-10-17 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * c-common.c (c_common_truthvalue_conversion): Warn only for signed > integer shift ops in boolean context. > > testsuite: > 2016-10-17 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * c-c++-common/Wint-in-bool-context-2.c: New test. Comment please in the code indicating why we're restricting this to signed shifts. I'm not entirely sure I agree with avoiding the warning for this case, but I'm not up for fighting about it. So OK after adding the comment. jeff
On 2016.10.19 at 14:13 -0600, Jeff Law wrote: > On 10/18/2016 12:14 PM, Bernd Edlinger wrote: > > On 10/18/16 19:05, Joseph Myers wrote: > > > > On Tue, 18 Oct 2016, Bernd Edlinger wrote: > > > > > > > > >> Hi, > > > > >> > > > > >> this restricts the -Wint-in-bool-context warning to signed shifts, > > > > >> to reduce the number of false positives Markus reported yesterday. > > > > > > > > This patch seems to be missing testcases (that warned before the patch > > > > and don't warn after it). > > > > > > Yes of course. > > > > New patch, this time with a test case, compiled from the linux sample. > > > > Bootstrapped and reg-tested as usual. > > Is it OK for trunk? > > > > > > Bernd. > > > > > > patch-bool-context4.diff > > > > > > c-family: > > 2016-10-17 Bernd Edlinger <bernd.edlinger@hotmail.de> > > > > * c-common.c (c_common_truthvalue_conversion): Warn only for signed > > integer shift ops in boolean context. > > > > testsuite: > > 2016-10-17 Bernd Edlinger <bernd.edlinger@hotmail.de> > > > > * c-c++-common/Wint-in-bool-context-2.c: New test. > Comment please in the code indicating why we're restricting this to signed > shifts. I'm not entirely sure I agree with avoiding the warning for this > case, but I'm not up for fighting about it. So OK after adding the comment. Thanks for the commit. But I think the comment is wrong: + /* We will only warn on unsigned shifts here, because the majority of ^^ This should be »signed«. -- Markus
c-family: 2016-10-17 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-common.c (c_common_truthvalue_conversion): Warn only for signed integer shift ops in boolean context. testsuite: 2016-10-17 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-c++-common/Wint-in-bool-context-2.c: New test. Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 241270) +++ gcc/c-family/c-common.c (working copy) @@ -3328,8 +3328,10 @@ TREE_OPERAND (expr, 0)); case LSHIFT_EXPR: - warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context, - "<< in boolean context, did you mean '<' ?"); + if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE + && !TYPE_UNSIGNED (TREE_TYPE (expr))) + warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context, + "<< in boolean context, did you mean '<' ?"); break; case COND_EXPR: Index: gcc/testsuite/c-c++-common/Wint-in-bool-context-2.c =================================================================== --- gcc/testsuite/c-c++-common/Wint-in-bool-context-2.c (revision 0) +++ gcc/testsuite/c-c++-common/Wint-in-bool-context-2.c (working copy) @@ -0,0 +1,17 @@ +/* { dg-options "-Wint-in-bool-context" } */ +/* { dg-do compile } */ + +typedef unsigned u32; +typedef unsigned char u8; +#define KEYLENGTH 8 + +int foo (u8 plen, u32 key) +{ + if ((plen < KEYLENGTH) && (key << plen)) /* { dg-bogus "boolean context" } */ + return -1; + + if ((plen << KEYLENGTH) && (key < plen)) /* { dg-warning "boolean context" } */ + return -2; + + return 0; +}