* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
@ 2016-09-02 15:51 Bernd Edlinger
2016-09-05 14:08 ` Marek Polacek
0 siblings, 1 reply; 17+ messages in thread
From: Bernd Edlinger @ 2016-09-02 15:51 UTC (permalink / raw)
To: Marek Polacek; +Cc: Joseph Myers, gcc-patches
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-02 15:51 C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses) Bernd Edlinger
@ 2016-09-05 14:08 ` Marek Polacek
2016-09-05 14:28 ` Bernd Edlinger
0 siblings, 1 reply; 17+ messages in thread
From: Marek Polacek @ 2016-09-05 14:08 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: Joseph Myers, gcc-patches
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 14:08 ` Marek Polacek
@ 2016-09-05 14:28 ` Bernd Edlinger
2016-09-05 14:39 ` Marek Polacek
2016-09-05 15:01 ` Eric Gallager
0 siblings, 2 replies; 17+ messages in thread
From: Bernd Edlinger @ 2016-09-05 14:28 UTC (permalink / raw)
To: Marek Polacek; +Cc: Joseph Myers, gcc-patches
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 14:28 ` Bernd Edlinger
@ 2016-09-05 14:39 ` Marek Polacek
2016-09-05 15:03 ` Bernd Edlinger
` (2 more replies)
2016-09-05 15:01 ` Eric Gallager
1 sibling, 3 replies; 17+ messages in thread
From: Marek Polacek @ 2016-09-05 14:39 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: Joseph Myers, gcc-patches
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 14:39 ` Marek Polacek
@ 2016-09-05 15:03 ` Bernd Edlinger
2016-09-05 16:46 ` Joseph Myers
2016-09-15 19:55 ` Jeff Law
2 siblings, 0 replies; 17+ messages in thread
From: Bernd Edlinger @ 2016-09-05 15:03 UTC (permalink / raw)
To: Marek Polacek; +Cc: Joseph Myers, gcc-patches
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 14:39 ` Marek Polacek
2016-09-05 15:03 ` Bernd Edlinger
@ 2016-09-05 16:46 ` Joseph Myers
2016-09-05 17:03 ` Bernd Edlinger
2016-09-15 19:55 ` Jeff Law
2 siblings, 1 reply; 17+ messages in thread
From: Joseph Myers @ 2016-09-05 16:46 UTC (permalink / raw)
To: Marek Polacek; +Cc: Bernd Edlinger, gcc-patches
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 16:46 ` Joseph Myers
@ 2016-09-05 17:03 ` Bernd Edlinger
0 siblings, 0 replies; 17+ messages in thread
From: Bernd Edlinger @ 2016-09-05 17:03 UTC (permalink / raw)
To: Joseph Myers, Marek Polacek; +Cc: gcc-patches
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 14:39 ` Marek Polacek
2016-09-05 15:03 ` Bernd Edlinger
2016-09-05 16:46 ` Joseph Myers
@ 2016-09-15 19:55 ` Jeff Law
2016-09-16 9:05 ` Marek Polacek
2 siblings, 1 reply; 17+ messages in thread
From: Jeff Law @ 2016-09-15 19:55 UTC (permalink / raw)
To: Marek Polacek, Bernd Edlinger; +Cc: Joseph Myers, gcc-patches
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-15 19:55 ` Jeff Law
@ 2016-09-16 9:05 ` Marek Polacek
0 siblings, 0 replies; 17+ messages in thread
From: Marek Polacek @ 2016-09-16 9:05 UTC (permalink / raw)
To: Jeff Law; +Cc: Bernd Edlinger, Joseph Myers, gcc-patches
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 14:28 ` Bernd Edlinger
2016-09-05 14:39 ` Marek Polacek
@ 2016-09-05 15:01 ` Eric Gallager
2016-09-05 15:08 ` Bernd Edlinger
1 sibling, 1 reply; 17+ messages in thread
From: Eric Gallager @ 2016-09-05 15:01 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: Marek Polacek, Joseph Myers, gcc-patches
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.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 15:01 ` Eric Gallager
@ 2016-09-05 15:08 ` Bernd Edlinger
0 siblings, 0 replies; 17+ messages in thread
From: Bernd Edlinger @ 2016-09-05 15:08 UTC (permalink / raw)
To: Eric Gallager; +Cc: Marek Polacek, Joseph Myers, gcc-patches
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
@ 2016-09-02 15:14 Marek Polacek
2016-09-05 10:51 ` Bernd Schmidt
0 siblings, 1 reply; 17+ messages in thread
From: Marek Polacek @ 2016-09-02 15:14 UTC (permalink / raw)
To: GCC Patches, Joseph Myers, Jason Merrill
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-02 15:14 Marek Polacek
@ 2016-09-05 10:51 ` Bernd Schmidt
2016-09-05 10:57 ` Marek Polacek
0 siblings, 1 reply; 17+ messages in thread
From: Bernd Schmidt @ 2016-09-05 10:51 UTC (permalink / raw)
To: Marek Polacek, GCC Patches, Joseph Myers, Jason Merrill
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 10:51 ` Bernd Schmidt
@ 2016-09-05 10:57 ` Marek Polacek
2016-09-05 16:12 ` Bernd Schmidt
0 siblings, 1 reply; 17+ messages in thread
From: Marek Polacek @ 2016-09-05 10:57 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: GCC Patches, Joseph Myers, Jason Merrill
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 10:57 ` Marek Polacek
@ 2016-09-05 16:12 ` Bernd Schmidt
2016-09-05 16:44 ` Sandra Loosemore
0 siblings, 1 reply; 17+ messages in thread
From: Bernd Schmidt @ 2016-09-05 16:12 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches, Joseph Myers, Jason Merrill, Sandra Loosemore
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 16:12 ` Bernd Schmidt
@ 2016-09-05 16:44 ` Sandra Loosemore
2016-09-06 9:08 ` Marek Polacek
0 siblings, 1 reply; 17+ messages in thread
From: Sandra Loosemore @ 2016-09-05 16:44 UTC (permalink / raw)
To: Bernd Schmidt, Marek Polacek; +Cc: GCC Patches, Joseph Myers, Jason Merrill
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses)
2016-09-05 16:44 ` Sandra Loosemore
@ 2016-09-06 9:08 ` Marek Polacek
0 siblings, 0 replies; 17+ messages in thread
From: Marek Polacek @ 2016-09-06 9:08 UTC (permalink / raw)
To: Sandra Loosemore; +Cc: Bernd Schmidt, GCC Patches, Joseph Myers, Jason Merrill
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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-09-16 9:03 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-02 15:51 C/C++ PATCH for c/77423 (bogus warning with -Wlogical-not-parentheses) Bernd Edlinger
2016-09-05 14:08 ` Marek Polacek
2016-09-05 14:28 ` Bernd Edlinger
2016-09-05 14:39 ` Marek Polacek
2016-09-05 15:03 ` Bernd Edlinger
2016-09-05 16:46 ` Joseph Myers
2016-09-05 17:03 ` Bernd Edlinger
2016-09-15 19:55 ` Jeff Law
2016-09-16 9:05 ` Marek Polacek
2016-09-05 15:01 ` Eric Gallager
2016-09-05 15:08 ` Bernd Edlinger
-- strict thread matches above, loose matches on Subject: below --
2016-09-02 15:14 Marek Polacek
2016-09-05 10:51 ` Bernd Schmidt
2016-09-05 10:57 ` Marek Polacek
2016-09-05 16:12 ` Bernd Schmidt
2016-09-05 16:44 ` Sandra Loosemore
2016-09-06 9:08 ` Marek Polacek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).