C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Marek Polacek]

Martin reported that -Wlogical-not-parentheses issues a bogus warning
for bit operations on booleans, because the warning didn't take integer
promotions into account.  The following patch ought to fix this problem.

It's not exactly trivial because I had to handle even more complex
expressions and comparison.  I am aware that given what I do with
CONVERT_EXPR_P in expr_has_boolean_operands_p, it's possible to construct
a pathological testcase where the C FE would warn, but C++ FE wouldn't.
But I'm not convinced it's worth complicating this code further.

I also fixed -Wlogical-not-parentheses wording in docs, following Martin's
suggestion.

Bootstrapped/regtested on x86_64-linux and ppc64-linux, ok for trunk?

2016-09-02  Marek Polacek  <polacek@redhat.com>

	PR c/77423
	* doc/invoke.texi: Update -Wlogical-not-parentheses documentation.

	* c-common.c (bool_promoted_to_int_p): New function.
	(expr_has_boolean_operands_p): New function.
	(warn_logical_not_parentheses): Return if expr_has_boolean_operands_p.
	(maybe_warn_bool_compare): Use bool_promoted_to_int_p.

	* c-c++-common/Wlogical-not-parentheses-3.c: New test.

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 399ba97..fbc8fb0 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -1479,6 +1479,36 @@ warn_tautological_cmp (location_t loc, enum tree_code code, tree lhs, tree rhs)
     }
 }
 
+/* Return true iff T is a boolean promoted to int.  */
+
+static bool
+bool_promoted_to_int_p (tree t)
+{
+  return (CONVERT_EXPR_P (t)
+	  && TREE_TYPE (t) == integer_type_node
+	  && TREE_CODE (TREE_TYPE (TREE_OPERAND (t, 0))) == BOOLEAN_TYPE);
+}
+
+/* Return true iff EXPR only contains boolean operands, or comparisons.  */
+
+static bool
+expr_has_boolean_operands_p (tree expr)
+{
+  STRIP_NOPS (expr);
+
+  if (CONVERT_EXPR_P (expr))
+    return bool_promoted_to_int_p (expr);
+  else if (UNARY_CLASS_P (expr))
+    return expr_has_boolean_operands_p (TREE_OPERAND (expr, 0));
+  else if (BINARY_CLASS_P (expr))
+    return (expr_has_boolean_operands_p (TREE_OPERAND (expr, 0))
+	    && expr_has_boolean_operands_p (TREE_OPERAND (expr, 1)));
+  else if (COMPARISON_CLASS_P (expr))
+    return true;
+  else
+    return false;
+}
+
 /* Warn about logical not used on the left hand side operand of a comparison.
    This function assumes that the LHS is inside of TRUTH_NOT_EXPR.
    Do not warn if RHS is of a boolean type, a logical operator, or
@@ -1494,6 +1524,10 @@ warn_logical_not_parentheses (location_t location, enum tree_code code,
       || truth_value_p (TREE_CODE (rhs)))
     return;
 
+  /* Don't warn for expression like !x == ~(bool1 | bool2).  */
+  if (expr_has_boolean_operands_p (rhs))
+    return;
+
   /* Don't warn for !x == 0 or !y != 0, those are equivalent to
      !(x == 0) or !(y != 0).  */
   if ((code == EQ_EXPR || code == NE_EXPR)
@@ -12405,9 +12439,7 @@ maybe_warn_bool_compare (location_t loc, enum tree_code code, tree op0,
 	 don't want to warn here.  */
       tree noncst = TREE_CODE (op0) == INTEGER_CST ? op1 : op0;
       /* Handle booleans promoted to integers.  */
-      if (CONVERT_EXPR_P (noncst)
-	  && TREE_TYPE (noncst) == integer_type_node
-	  && TREE_CODE (TREE_TYPE (TREE_OPERAND (noncst, 0))) == BOOLEAN_TYPE)
+      if (bool_promoted_to_int_p (noncst))
 	/* Warn.  */;
       else if (TREE_CODE (TREE_TYPE (noncst)) != BOOLEAN_TYPE
 	       && !truth_value_p (TREE_CODE (noncst)))
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 87da1f1..38d55d4 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -5437,8 +5437,8 @@ if (a < 0 && a < 0) @{ @dots{} @}
 @opindex Wlogical-not-parentheses
 @opindex Wno-logical-not-parentheses
 Warn about logical not used on the left hand side operand of a comparison.
-This option does not warn if the RHS operand is of a boolean type.  Its
-purpose is to detect suspicious code like the following:
+This option does not warn if the right operand is considered to be a Boolean
+expression.  Its purpose is to detect suspicious code like the following:
 @smallexample
 int a;
 @dots{}
diff --git gcc/testsuite/c-c++-common/Wlogical-not-parentheses-3.c gcc/testsuite/c-c++-common/Wlogical-not-parentheses-3.c
index e69de29..00aa747 100644
--- gcc/testsuite/c-c++-common/Wlogical-not-parentheses-3.c
+++ gcc/testsuite/c-c++-common/Wlogical-not-parentheses-3.c
@@ -0,0 +1,31 @@
+/* PR c/77423 */
+/* { dg-do compile } */
+/* { dg-options "-Wlogical-not-parentheses" } */
+
+#ifndef __cplusplus
+# define bool _Bool
+#endif
+
+int
+f (int a, bool b, bool c)
+{
+  int r = 0;
+
+  r += !a == (b | c);
+  r += !a == (b ^ c);
+  r += !a == (b & c);
+  r += !a == ~b;
+  r += !a == ~(int) b;
+  r += !a == ((b & c) | c);
+  r += !a == ((b & c) | (b ^ c));
+  r += !a == (int) (b ^ c);
+  r += !a == (int) ~b;
+  r += !a == ~~b;
+  r += !a == ~(b | c);
+  r += !a == ~(b | (a == 1));
+  r += !a == ~(a == 1);
+
+  r += !a == ((b & c) | (b ^ a)); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+
+  return r;
+}

	Marek

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Bernd Schmidt]

On 09/02/2016 05:13 PM, Marek Polacek wrote:
> diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
> index 87da1f1..38d55d4 100644
> --- gcc/doc/invoke.texi
> +++ gcc/doc/invoke.texi
> @@ -5437,8 +5437,8 @@ if (a < 0 && a < 0) @{ @dots{} @}
>  @opindex Wlogical-not-parentheses
>  @opindex Wno-logical-not-parentheses
>  Warn about logical not used on the left hand side operand of a comparison.
> -This option does not warn if the RHS operand is of a boolean type.  Its
> -purpose is to detect suspicious code like the following:
> +This option does not warn if the right operand is considered to be a Boolean
> +expression.  Its purpose is to detect suspicious code like the following:

I think "Boolean" shouldn't be capitalized. The patch looks ok to me 
otherwise.

> +  r += !a == (b | c);

I do wonder whether we should give a different warning for this though. 
I personally prefer code involving bools to use || to show it's a 
logical operation. Bit-wise may indicate mistakes where the programmer 
thought he was operating on integers.


Bernd

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Marek Polacek]

On Mon, Sep 05, 2016 at 12:35:13PM +0200, Bernd Schmidt wrote:
> On 09/02/2016 05:13 PM, Marek Polacek wrote:
> > diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
> > index 87da1f1..38d55d4 100644
> > --- gcc/doc/invoke.texi
> > +++ gcc/doc/invoke.texi
> > @@ -5437,8 +5437,8 @@ if (a < 0 && a < 0) @{ @dots{} @}
> >  @opindex Wlogical-not-parentheses
> >  @opindex Wno-logical-not-parentheses
> >  Warn about logical not used on the left hand side operand of a comparison.
> > -This option does not warn if the RHS operand is of a boolean type.  Its
> > -purpose is to detect suspicious code like the following:
> > +This option does not warn if the right operand is considered to be a Boolean
> > +expression.  Its purpose is to detect suspicious code like the following:
> 
> I think "Boolean" shouldn't be capitalized. The patch looks ok to me
> otherwise.
 
No strong opinions, but looking at
https://en.wikipedia.org/wiki/Boolean_expression
I see that it's capitalized there, so I think let's keep "Boolean".

> > +  r += !a == (b | c);
> 
> I do wonder whether we should give a different warning for this though. I
> personally prefer code involving bools to use || to show it's a logical
> operation. Bit-wise may indicate mistakes where the programmer thought he
> was operating on integers.

Yea.  I'd swear that I've seen an RFE somewhere for this in the GCC BZ,
I mean to warn for bool1 * bool2, bool1 / bool2, etc.  Though I'm not sure
if warning for bool1 | bool2 would be desirable.

Thanks, I think I'll commit this later today.

	Marek

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Bernd Schmidt]

On 09/05/2016 12:52 PM, Marek Polacek wrote:
> On Mon, Sep 05, 2016 at 12:35:13PM +0200, Bernd Schmidt wrote:
>> On 09/02/2016 05:13 PM, Marek Polacek wrote:
>>> diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
>>> index 87da1f1..38d55d4 100644
>>> --- gcc/doc/invoke.texi
>>> +++ gcc/doc/invoke.texi
>>> @@ -5437,8 +5437,8 @@ if (a < 0 && a < 0) @{ @dots{} @}
>>>  @opindex Wlogical-not-parentheses
>>>  @opindex Wno-logical-not-parentheses
>>>  Warn about logical not used on the left hand side operand of a comparison.
>>> -This option does not warn if the RHS operand is of a boolean type.  Its
>>> -purpose is to detect suspicious code like the following:
>>> +This option does not warn if the right operand is considered to be a Boolean
>>> +expression.  Its purpose is to detect suspicious code like the following:
>>
>> I think "Boolean" shouldn't be capitalized. The patch looks ok to me
>> otherwise.
>
> No strong opinions, but looking at
> https://en.wikipedia.org/wiki/Boolean_expression
> I see that it's capitalized there, so I think let's keep "Boolean".

I looked at other occurrences in this file, and they are lower-case. 
Across the other texi files upper-case is also the exception. Let's ask 
Sandra for a ruling?


Bernd

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Sandra Loosemore]

On 09/05/2016 09:55 AM, Bernd Schmidt wrote:
>
>
> On 09/05/2016 12:52 PM, Marek Polacek wrote:
>> On Mon, Sep 05, 2016 at 12:35:13PM +0200, Bernd Schmidt wrote:
>>> On 09/02/2016 05:13 PM, Marek Polacek wrote:
>>>> diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
>>>> index 87da1f1..38d55d4 100644
>>>> --- gcc/doc/invoke.texi
>>>> +++ gcc/doc/invoke.texi
>>>> @@ -5437,8 +5437,8 @@ if (a < 0 && a < 0) @{ @dots{} @}
>>>>  @opindex Wlogical-not-parentheses
>>>>  @opindex Wno-logical-not-parentheses
>>>>  Warn about logical not used on the left hand side operand of a
>>>> comparison.
>>>> -This option does not warn if the RHS operand is of a boolean type.
>>>> Its
>>>> -purpose is to detect suspicious code like the following:
>>>> +This option does not warn if the right operand is considered to be
>>>> a Boolean
>>>> +expression.  Its purpose is to detect suspicious code like the
>>>> following:
>>>
>>> I think "Boolean" shouldn't be capitalized. The patch looks ok to me
>>> otherwise.
>>
>> No strong opinions, but looking at
>> https://en.wikipedia.org/wiki/Boolean_expression
>> I see that it's capitalized there, so I think let's keep "Boolean".
>
> I looked at other occurrences in this file, and they are lower-case.
> Across the other texi files upper-case is also the exception. Let's ask
> Sandra for a ruling?

Ummmm.  There seems to be precedent for both capitalizing it and not.

The C standard doesn't capitalize it (but the only place I could find 
where "boolean" used in a context where it wouldn't be capitalized 
anyway, such as the first word in a section title, is the index).  The 
C++ standard isn't consistent, using both "Boolean" and "boolean" (and 
sometimes even on the same page).  The documents I have handy are older 
drafts, though; maybe that has been cleaned up in current/final versions.

Looking at some other languages, the Python documentation capitalizes:

https://docs.python.org/3/library/stdtypes.html

But the Scheme specification editors deliberately chose not to do so:

http://www.r6rs.org/r6rs-editors/2007-April/002143.html

The Haskell specification also uses lowercase:

https://www.haskell.org/onlinereport/haskell2010/haskellch6.html#x13-1170006.1

Mark-Jason Dominus gave the matter some serious thought:

http://blog.plover.com/lang/Boolean.html

Anyway, I will be happy either way as long as the documentation is 
consistent.

-Sandra

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Marek Polacek]

On Mon, Sep 05, 2016 at 10:35:26AM -0600, Sandra Loosemore wrote:
> On 09/05/2016 09:55 AM, Bernd Schmidt wrote:
> > 
> > 
> > On 09/05/2016 12:52 PM, Marek Polacek wrote:
> > > On Mon, Sep 05, 2016 at 12:35:13PM +0200, Bernd Schmidt wrote:
> > > > On 09/02/2016 05:13 PM, Marek Polacek wrote:
> > > > > diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
> > > > > index 87da1f1..38d55d4 100644
> > > > > --- gcc/doc/invoke.texi
> > > > > +++ gcc/doc/invoke.texi
> > > > > @@ -5437,8 +5437,8 @@ if (a < 0 && a < 0) @{ @dots{} @}
> > > > >  @opindex Wlogical-not-parentheses
> > > > >  @opindex Wno-logical-not-parentheses
> > > > >  Warn about logical not used on the left hand side operand of a
> > > > > comparison.
> > > > > -This option does not warn if the RHS operand is of a boolean type.
> > > > > Its
> > > > > -purpose is to detect suspicious code like the following:
> > > > > +This option does not warn if the right operand is considered to be
> > > > > a Boolean
> > > > > +expression.  Its purpose is to detect suspicious code like the
> > > > > following:
> > > > 
> > > > I think "Boolean" shouldn't be capitalized. The patch looks ok to me
> > > > otherwise.
> > > 
> > > No strong opinions, but looking at
> > > https://en.wikipedia.org/wiki/Boolean_expression
> > > I see that it's capitalized there, so I think let's keep "Boolean".
> > 
> > I looked at other occurrences in this file, and they are lower-case.
> > Across the other texi files upper-case is also the exception. Let's ask
> > Sandra for a ruling?
> 
> Ummmm.  There seems to be precedent for both capitalizing it and not.
> 
> The C standard doesn't capitalize it (but the only place I could find where
> "boolean" used in a context where it wouldn't be capitalized anyway, such as
> the first word in a section title, is the index).  The C++ standard isn't
> consistent, using both "Boolean" and "boolean" (and sometimes even on the
> same page).  The documents I have handy are older drafts, though; maybe that
> has been cleaned up in current/final versions.
 
Thanks for the research!

> Looking at some other languages, the Python documentation capitalizes:
> 
> https://docs.python.org/3/library/stdtypes.html
> 
> But the Scheme specification editors deliberately chose not to do so:
> 
> http://www.r6rs.org/r6rs-editors/2007-April/002143.html
> 
> The Haskell specification also uses lowercase:
> 
> https://www.haskell.org/onlinereport/haskell2010/haskellch6.html#x13-1170006.1
> 
> Mark-Jason Dominus gave the matter some serious thought:
> 
> http://blog.plover.com/lang/Boolean.html
 
Ok, I buy that.

> Anyway, I will be happy either way as long as the documentation is
> consistent.

Let's go with lowercase then.  I'll post a cleanup patch.

	Marek

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Marek Polacek]

On Thu, Sep 15, 2016 at 01:21:21PM -0600, Jeff Law wrote:
> On 09/05/2016 08:28 AM, Marek Polacek wrote:
> > > test.c:10:8: note: add parentheses around left hand side expression to
> > > silence this warning
> > >     r += !a == ~b;
> > >          ^~
> > >          ( )
> > > 
> > > this will not fix it, but make  it worse.
> > > I think a better warning would be
> > > warning: ~ on boolean value, did you mean ! ?
> > 
> > Could you please open a PR?  I'll take care of it.
> > 
> > Still not sure about other operations.  I guess no one would
> > object to warning on bool1 % bool2, but should we warn for
> > bool1 + bool2?
> Wouldn't the desire for a warning largely depend on the type of the result?
> So I'll assume you're referring to a boolean result :-)
> 
> bool1 + bool2 does have meaning though, even when the result is a bool. You
> have to be leery of both having a true value as that causes an overflow.

Yea.  The version of the patch I posted doesn't warn for addition of booleans.

	Marek

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Jeff Law]

On 09/05/2016 08:28 AM, Marek Polacek wrote:
>> test.c:10:8: note: add parentheses around left hand side expression to
>> silence this warning
>>     r += !a == ~b;
>>          ^~
>>          ( )
>>
>> this will not fix it, but make  it worse.
>> I think a better warning would be
>> warning: ~ on boolean value, did you mean ! ?
>
> Could you please open a PR?  I'll take care of it.
>
> Still not sure about other operations.  I guess no one would
> object to warning on bool1 % bool2, but should we warn for
> bool1 + bool2?
Wouldn't the desire for a warning largely depend on the type of the 
result?  So I'll assume you're referring to a boolean result :-)

bool1 + bool2 does have meaning though, even when the result is a bool. 
You have to be leery of both having a true value as that causes an overflow.

Jeff

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Bernd Edlinger]

On 09/05/16 18:45, Joseph Myers wrote:
> On Mon, 5 Sep 2016, Marek Polacek wrote:
>
>> Still not sure about other operations.  I guess no one would
>> object to warning on bool1 % bool2, but should we warn for
>> bool1 + bool2?
>
> I think boolean addition (with the result interpreted as an integer, not
> converted back to boolean) is perfectly reasonable - counting the number
> of flags that are true, for example (say if there are several conditions
> and it's an error for more than one of them to hold - of course that would
> be bool1 + bool2 + bool3 + bool4, etc.).
>

Yes, that's what I had in mind too.  I think I even remember having seen
code like this, which is OK as long as the result is stored in an
integer variable.

But "if (bool1 + bool2)" should be written as "if (bool1 | bool2)",
and "if (bool1 * bool2)" should be written as "if (bool1 & bool2)".

I think a warning for boolean + and * suggesting to use | and &
is justified for clarity.


Bernd.

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Joseph Myers]

On Mon, 5 Sep 2016, Marek Polacek wrote:

> Still not sure about other operations.  I guess no one would
> object to warning on bool1 % bool2, but should we warn for
> bool1 + bool2?

I think boolean addition (with the result interpreted as an integer, not 
converted back to boolean) is perfectly reasonable - counting the number 
of flags that are true, for example (say if there are several conditions 
and it's an error for more than one of them to hold - of course that would 
be bool1 + bool2 + bool3 + bool4, etc.).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Bernd Edlinger]

On 09/05/16 17:00, Eric Gallager wrote:
> On 9/5/16, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>> On 09/05/16 14:03, Marek Polacek wrote:
>>> On Fri, Sep 02, 2016 at 03:51:49PM +0000, Bernd Edlinger wrote:
>>>> Hi,
>>>>
>>>>    > +  r += !a == ~b;
>>>>    > +  r += !a == ~(int) b;
>>>>
>>>> I don't understand why ~b should not be warned at -Wall.
>>>
>>> Yeah, there was an RFE for this but I'm not finding it.
>>>
>>
>> I certainly agree that the warning is compeletely misleading here:
>>
>> test.c: In function 'f':
>> test.c:10:11: warning: logical not is only applied to the left hand side
>> of comparison [-Wlogical-not-parentheses]
>>      r += !a == ~b;
>>              ^~
>> test.c:10:8: note: add parentheses around left hand side expression to
>> silence this warning
>>      r += !a == ~b;
>>           ^~
>>           ( )
>>
>> this will not fix it, but make  it worse.
>> I think a better warning would be
>> warning: ~ on boolean value, did you mean ! ?
>>
>
>
> The exclamation mark followed by question mark looks kind of confusing
> to me; it looks too much like punctuation (instead of an operator).
> Maybe quote it or spell it out?
> i.e.
> warning: '~' on boolean value, did you mean '!' ?
> or
> warning: tilde used to negate boolean value, did you mean to use an
> exclamation mark to negate it instead?
>
>

Yes, thanks good point.

Bernd.

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Bernd Edlinger]

On 09/05/16 16:28, Marek Polacek wrote:
> On Mon, Sep 05, 2016 at 02:08:20PM +0000, Bernd Edlinger wrote:
>> On 09/05/16 14:03, Marek Polacek wrote:
>>> On Fri, Sep 02, 2016 at 03:51:49PM +0000, Bernd Edlinger wrote:
>>>> Hi,
>>>>
>>>>    > +  r += !a == ~b;
>>>>    > +  r += !a == ~(int) b;
>>>>
>>>> I don't understand why ~b should not be warned at -Wall.
>>>
>>> Yeah, there was an RFE for this but I'm not finding it.
>>>
>>
>> I certainly agree that the warning is compeletely misleading here:
>>
>> test.c: In function 'f':
>> test.c:10:11: warning: logical not is only applied to the left hand side
>> of comparison [-Wlogical-not-parentheses]
>>      r += !a == ~b;
>>              ^~
>> test.c:10:8: note: add parentheses around left hand side expression to
>> silence this warning
>>      r += !a == ~b;
>>           ^~
>>           ( )
>>
>> this will not fix it, but make  it worse.
>> I think a better warning would be
>> warning: ~ on boolean value, did you mean ! ?
>
> Could you please open a PR?  I'll take care of it.
>

Done, thanks: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77490

> Still not sure about other operations.  I guess no one would
> object to warning on bool1 % bool2, but should we warn for
> bool1 + bool2?
>

Yeah, any % bool is just blandly insane.  Please add a warning for that
as well.

Not totally sure what to do about bool + bool: If it is *not* used in 
boolean context it's probably OK, but in boolean context why not use |
instead?

Note: we even optimize unary - in boolean context:
see c-family/c-common.c (c_common_truthvalue_conversion):

     case NEGATE_EXPR:
     case ABS_EXPR:
     case FLOAT_EXPR:
     case EXCESS_PRECISION_EXPR:
       /* These don't change whether an object is nonzero or zero.  */
       return c_common_truthvalue_conversion (location, TREE_OPERAND 
(expr, 0));


I wonder why there is no warning for NEGATE_EXPR and ABS_EXPR.


Bernd.

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Eric Gallager]

On 9/5/16, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> On 09/05/16 14:03, Marek Polacek wrote:
>> On Fri, Sep 02, 2016 at 03:51:49PM +0000, Bernd Edlinger wrote:
>>> Hi,
>>>
>>>   > +  r += !a == ~b;
>>>   > +  r += !a == ~(int) b;
>>>
>>> I don't understand why ~b should not be warned at -Wall.
>>
>> Yeah, there was an RFE for this but I'm not finding it.
>>
>
> I certainly agree that the warning is compeletely misleading here:
>
> test.c: In function 'f':
> test.c:10:11: warning: logical not is only applied to the left hand side
> of comparison [-Wlogical-not-parentheses]
>     r += !a == ~b;
>             ^~
> test.c:10:8: note: add parentheses around left hand side expression to
> silence this warning
>     r += !a == ~b;
>          ^~
>          ( )
>
> this will not fix it, but make  it worse.
> I think a better warning would be
> warning: ~ on boolean value, did you mean ! ?
>


The exclamation mark followed by question mark looks kind of confusing
to me; it looks too much like punctuation (instead of an operator).
Maybe quote it or spell it out?
i.e.
warning: '~' on boolean value, did you mean '!' ?
or
warning: tilde used to negate boolean value, did you mean to use an
exclamation mark to negate it instead?


>
> Index: gcc/c/c-typeck.c
> ===================================================================
> --- gcc/c/c-typeck.c    (revision 239971)
> +++ gcc/c/c-typeck.c    (working copy)
> @@ -4192,6 +4192,17 @@ build_unary_op (location_t location,
>         break;
>
>       case BIT_NOT_EXPR:
> +     {
> +       tree e = arg;
> +
> +       /* Warn if the condition has boolean value.  */
> +       while (TREE_CODE (e) == COMPOUND_EXPR)
> +         e = TREE_OPERAND (e, 1);
> +
> +       if (TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE
> +           || truth_value_p (TREE_CODE (e)))
> +         warning(0, "~ on boolean value, did you mean ! ?");
> +      }
>         /* ~ works on integer types and non float vectors. */
>         if (typecode == INTEGER_TYPE
>            || (typecode == VECTOR_TYPE
> Index: gcc/cp/typeck.c
> ===================================================================
> --- gcc/cp/typeck.c     (revision 239971)
> +++ gcc/cp/typeck.c     (working copy)
> @@ -5839,6 +5839,8 @@ cp_build_unary_op (enum tree_code code, tree xarg,
>         break;
>
>       case BIT_NOT_EXPR:
> +      if (TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE)
> +       warning (0, "~ on boolean value, did you mean ! ?");
>         if (TREE_CODE (TREE_TYPE (arg)) == COMPLEX_TYPE)
>          {
>            code = CONJ_EXPR;
>
>
>
> Bernd.
>

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Marek Polacek]

On Mon, Sep 05, 2016 at 02:08:20PM +0000, Bernd Edlinger wrote:
> On 09/05/16 14:03, Marek Polacek wrote:
> > On Fri, Sep 02, 2016 at 03:51:49PM +0000, Bernd Edlinger wrote:
> >> Hi,
> >>
> >>   > +  r += !a == ~b;
> >>   > +  r += !a == ~(int) b;
> >>
> >> I don't understand why ~b should not be warned at -Wall.
> >
> > Yeah, there was an RFE for this but I'm not finding it.
> >
> 
> I certainly agree that the warning is compeletely misleading here:
> 
> test.c: In function 'f':
> test.c:10:11: warning: logical not is only applied to the left hand side 
> of comparison [-Wlogical-not-parentheses]
>     r += !a == ~b;
>             ^~
> test.c:10:8: note: add parentheses around left hand side expression to 
> silence this warning
>     r += !a == ~b;
>          ^~
>          ( )
> 
> this will not fix it, but make  it worse.
> I think a better warning would be
> warning: ~ on boolean value, did you mean ! ?
 
Could you please open a PR?  I'll take care of it.

Still not sure about other operations.  I guess no one would
object to warning on bool1 % bool2, but should we warn for
bool1 + bool2?

	Marek

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Bernd Edlinger]

On 09/05/16 14:03, Marek Polacek wrote:
> On Fri, Sep 02, 2016 at 03:51:49PM +0000, Bernd Edlinger wrote:
>> Hi,
>>
>>   > +  r += !a == ~b;
>>   > +  r += !a == ~(int) b;
>>
>> I don't understand why ~b should not be warned at -Wall.
>
> Yeah, there was an RFE for this but I'm not finding it.
>

I certainly agree that the warning is compeletely misleading here:

test.c: In function 'f':
test.c:10:11: warning: logical not is only applied to the left hand side 
of comparison [-Wlogical-not-parentheses]
    r += !a == ~b;
            ^~
test.c:10:8: note: add parentheses around left hand side expression to 
silence this warning
    r += !a == ~b;
         ^~
         ( )

this will not fix it, but make  it worse.
I think a better warning would be
warning: ~ on boolean value, did you mean ! ?

>> Frankly I don't even understand why the above statements are
>> completely optimized away.
>>
>> r += !a == ~b;
>> is optimized away, but
>>
>> b = ~b;
>> r += !a == b;
>>
>> Is not.  Why?
>
> Something in ccp I suppose.  But I didn't investigate closely because
> it's not really relevant to this patch, sorry.
>

Yes, I think the problem is that ~ on bool maps [0:1] to [-1:-2]
and thus the result is no longer boolean.  But probaby the assignment
normalizes the result again: b = ~b; has the effect of setting b = 1.

A warning for "~ on boolean" would not be too difficult, but making a
fix-it will probably need more work:

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c    (revision 239971)
+++ gcc/c/c-typeck.c    (working copy)
@@ -4192,6 +4192,17 @@ build_unary_op (location_t location,
        break;

      case BIT_NOT_EXPR:
+     {
+       tree e = arg;
+
+       /* Warn if the condition has boolean value.  */
+       while (TREE_CODE (e) == COMPOUND_EXPR)
+         e = TREE_OPERAND (e, 1);
+
+       if (TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE
+           || truth_value_p (TREE_CODE (e)))
+         warning(0, "~ on boolean value, did you mean ! ?");
+      }
        /* ~ works on integer types and non float vectors. */
        if (typecode == INTEGER_TYPE
           || (typecode == VECTOR_TYPE
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c     (revision 239971)
+++ gcc/cp/typeck.c     (working copy)
@@ -5839,6 +5839,8 @@ cp_build_unary_op (enum tree_code code, tree xarg,
        break;

      case BIT_NOT_EXPR:
+      if (TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE)
+       warning (0, "~ on boolean value, did you mean ! ?");
        if (TREE_CODE (TREE_TYPE (arg)) == COMPLEX_TYPE)
         {
           code = CONJ_EXPR;



Bernd.

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Marek Polacek]

On Fri, Sep 02, 2016 at 03:51:49PM +0000, Bernd Edlinger wrote:
> Hi,
> 
>  > +  r += !a == ~b;
>  > +  r += !a == ~(int) b;
> 
> I don't understand why ~b should not be warned at -Wall.
 
Yeah, there was an RFE for this but I'm not finding it.

> Frankly I don't even understand why the above statements are
> completely optimized away.
> 
> r += !a == ~b;
> is optimized away, but
> 
> b = ~b;
> r += !a == b;
> 
> Is not.  Why?

Something in ccp I suppose.  But I didn't investigate closely because
it's not really relevant to this patch, sorry.

	Marek

Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)

[Bernd Edlinger]

Hi,

 > +  r += !a == ~b;
 > +  r += !a == ~(int) b;

I don't understand why ~b should not be warned at -Wall.

Frankly I don't even understand why the above statements are
completely optimized away.

r += !a == ~b;
is optimized away, but

b = ~b;
r += !a == b;

Is not.  Why?



Bernd.