diff mbox

Extend -Wint-in-bool-context to warn for multiplications

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

Commit Message

Bernd Edlinger Oct. 23, 2016, 11:31 a.m. UTC 
On 10/22/16 08:52, Bernd Edlinger wrote:
> On 10/22/16 04:17, Martin Sebor wrote:
>> On 10/21/2016 04:37 PM, Joseph Myers wrote:
>>> The quoting in the diagnostic should be %<&&%>, not '&&'.
>>
>> Presumably same for '*' (i.e., %<*%>).
>>
>> But I would actually suggest a somewhat more formal phrasing than
>> "better use xxx here" such as "suggest %<&&%> instead" or something
>> akin to what's already in place elsewhere in gcc.pot.
>>
>
> Aehm, yes.  That would be better then:
>
>
> Index: c-common.c
> ===================================================================
> --- c-common.c    (revision 241400)
> +++ c-common.c    (working copy)
> @@ -3327,6 +3327,11 @@
>      return c_common_truthvalue_conversion (location,
>                             TREE_OPERAND (expr, 0));
>
> +    case MULT_EXPR:
> +      warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
> +          "%<*%> in boolean context, suggest %<&&%> instead");
> +      break;
> +
>      case LSHIFT_EXPR:
>        /* We will only warn on signed shifts here, because the majority of
>       false positive warnings happen in code where unsigned arithmetic
>
>
> I assume then I should adjust the warning a few lines below as well:
>
>         warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
>                     "<< in boolean context, did you mean '<' ?");
>
>

Attached is the updated patch with those quotes fixed.

I have now put the << and < in correct quotes, but left the ?: in
the next two warnings unquoted:

  "?: using integer constants in boolean context, "
  "the expression will always evaluate to %<true%>"

I copied that style from the warning about omitted middle operand of
conditional expressions:

"the omitted middle operand in ?: will always be %<true%>, suggest 
explicit "
"middle operand"

I think that could be explained because ?: is not really a keyword
like <<, and is just a shorter phrase than "conditional expression".


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

Comments

Jeff Law Oct. 24, 2016, 4:13 p.m. UTC | #1 
On 10/23/2016 05:31 AM, Bernd Edlinger wrote:
> On 10/22/16 08:52, Bernd Edlinger wrote:
>> > On 10/22/16 04:17, Martin Sebor wrote:
>>> >> On 10/21/2016 04:37 PM, Joseph Myers wrote:
>>>> >>> The quoting in the diagnostic should be %<&&%>, not '&&'.
>>> >>
>>> >> Presumably same for '*' (i.e., %<*%>).
>>> >>
>>> >> But I would actually suggest a somewhat more formal phrasing than
>>> >> "better use xxx here" such as "suggest %<&&%> instead" or something
>>> >> akin to what's already in place elsewhere in gcc.pot.
>>> >>
>> >
>> > Aehm, yes.  That would be better then:
>> >
>> >
>> > Index: c-common.c
>> > ===================================================================
>> > --- c-common.c    (revision 241400)
>> > +++ c-common.c    (working copy)
>> > @@ -3327,6 +3327,11 @@
>> >      return c_common_truthvalue_conversion (location,
>> >                             TREE_OPERAND (expr, 0));
>> >
>> > +    case MULT_EXPR:
>> > +      warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
>> > +          "%<*%> in boolean context, suggest %<&&%> instead");
>> > +      break;
>> > +
>> >      case LSHIFT_EXPR:
>> >        /* We will only warn on signed shifts here, because the majority of
>> >       false positive warnings happen in code where unsigned arithmetic
>> >
>> >
>> > I assume then I should adjust the warning a few lines below as well:
>> >
>> >         warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
>> >                     "<< in boolean context, did you mean '<' ?");
>> >
>> >
> Attached is the updated patch with those quotes fixed.
>
> I have now put the << and < in correct quotes, but left the ?: in
> the next two warnings unquoted:
>
>   "?: using integer constants in boolean context, "
>   "the expression will always evaluate to %<true%>"
>
> I copied that style from the warning about omitted middle operand of
> conditional expressions:
>
> "the omitted middle operand in ?: will always be %<true%>, suggest
> explicit "
> "middle operand"
>
> I think that could be explained because ?: is not really a keyword
> like <<, and is just a shorter phrase than "conditional expression".
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
>
> Thanks
> Bernd.
>
>
> patch-bool-context5.diff
>
>
> c-family:
> 2016-10-23  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	* c-common.c (c_common_truthvalue_conversion): Warn for
> 	multiplications in boolean context.  Fix the quoting of '<<' and '<'
> 	in the shift warning.
>
> gcc:
> 2016-10-23  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	* doc/invoke.text (Wint-in-bool-context): Update documentation.
> 	* value-prof.c (stringop_block_profile): Fix a warning.
>
> testsuite:
> 2016-10-23  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	* c-c++-common/Wint-in-bool-context-3.c: New test.
OK.

Jeff
diff mbox

Patch

c-family:
2016-10-23  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-common.c (c_common_truthvalue_conversion): Warn for
	multiplications in boolean context.  Fix the quoting of '<<' and '<'
	in the shift warning.

gcc:
2016-10-23  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/invoke.text (Wint-in-bool-context): Update documentation.
	* value-prof.c (stringop_block_profile): Fix a warning.

testsuite:
2016-10-23  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/Wint-in-bool-context-3.c: New test.

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 241437)
+++ gcc/c-family/c-common.c	(working copy)
@@ -3327,6 +3327,11 @@  c_common_truthvalue_conversion (location_t locatio
 	return c_common_truthvalue_conversion (location,
 					       TREE_OPERAND (expr, 0));
 
+    case MULT_EXPR:
+      warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
+		  "%<*%> in boolean context, suggest %<&&%> instead");
+      break;
+
     case LSHIFT_EXPR:
       /* We will only warn on signed shifts here, because the majority of
 	 false positive warnings happen in code where unsigned arithmetic
@@ -3336,7 +3341,7 @@  c_common_truthvalue_conversion (location_t locatio
       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 '<' ?");
+		    "%<<<%> in boolean context, did you mean %<<%> ?");
       break;
 
     case COND_EXPR:
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 241437)
+++ gcc/doc/invoke.texi	(working copy)
@@ -6169,8 +6169,9 @@  of the C++ standard.
 @opindex Wno-int-in-bool-context
 Warn for suspicious use of integer values where boolean values are expected,
 such as conditional expressions (?:) using non-boolean integer constants in
-boolean context, like @code{if (a <= b ? 2 : 3)}.  Or left shifting in
-boolean context, like @code{for (a = 0; 1 << a; a++);}.
+boolean context, like @code{if (a <= b ? 2 : 3)}.  Or left shifting of signed
+integers in boolean context, like @code{for (a = 0; 1 << a; a++);}.  Likewise
+for all kinds of multiplications regardless of the data type.
 This warning is enabled by @option{-Wall}.
 
 @item -Wno-int-to-pointer-cast
Index: gcc/value-prof.c
===================================================================
--- gcc/value-prof.c	(revision 241437)
+++ gcc/value-prof.c	(working copy)
@@ -1878,12 +1878,12 @@  stringop_block_profile (gimple *stmt, unsigned int
   else
     {
       gcov_type count;
-      int alignment;
+      unsigned int alignment;
 
       count = histogram->hvalue.counters[0];
       alignment = 1;
       while (!(count & alignment)
-	     && (alignment * 2 * BITS_PER_UNIT))
+	     && (alignment <= UINT_MAX / 2 / BITS_PER_UNIT))
 	alignment <<= 1;
       *expected_align = alignment * BITS_PER_UNIT;
       gimple_remove_histogram_value (cfun, stmt, histogram);
Index: gcc/testsuite/c-c++-common/Wint-in-bool-context-3.c
===================================================================
--- gcc/testsuite/c-c++-common/Wint-in-bool-context-3.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wint-in-bool-context-3.c	(working copy)
@@ -0,0 +1,15 @@ 
+/* { dg-options "-Wint-in-bool-context" } */
+/* { dg-do compile } */
+
+#define BITS_PER_UNIT 8
+
+int foo (int count)
+{
+  int alignment;
+ 
+  alignment = 1;
+  while (!(count & alignment)
+         && (alignment * 2 * BITS_PER_UNIT)) /* { dg-warning "boolean context" } */
+    alignment <<= 1;
+  return alignment * BITS_PER_UNIT;
+}