diff mbox

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

Message ID 20160902151352.GH3768@redhat.com
State New
Headers show

Commit Message

Marek Polacek Sept. 2, 2016, 3:13 p.m. UTC 
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.


	Marek

Comments

Bernd Schmidt Sept. 5, 2016, 10:35 a.m. UTC | #1 
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
Marek Polacek Sept. 5, 2016, 10:52 a.m. UTC | #2 
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
Bernd Schmidt Sept. 5, 2016, 3:55 p.m. UTC | #3 
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
Sandra Loosemore Sept. 5, 2016, 4:35 p.m. UTC | #4 
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
Marek Polacek Sept. 6, 2016, 9:08 a.m. UTC | #5 
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
diff mbox

Patch

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