diff mbox

Make -Wint-in-bool-context warn on suspicious shift ops

Message ID AM4PR0701MB2162FAB66D1F87E5197975AAE4D30@AM4PR0701MB2162.eurprd07.prod.outlook.com
State New
Headers show

Commit Message

Bernd Edlinger Oct. 18, 2016, 6:14 p.m. UTC 
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.

Comments

Jeff Law Oct. 19, 2016, 8:13 p.m. UTC | #1 
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
Markus Trippelsdorf Oct. 20, 2016, 8:05 a.m. UTC | #2 
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«.
diff mbox

Patch

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;
+}