[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

Andrew Pinski  changed:

   What|Removed |Added

 Status|UNCONFIRMED |RESOLVED
 Resolution|--- |FIXED
   Target Milestone|--- |7.0

--- Comment #13 from Andrew Pinski  ---
Fixed.

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[edlinger at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

--- Comment #12 from Bernd Edlinger  ---
Author: edlinger
Date: Mon Sep 19 22:10:11 2016
New Revision: 240251

URL: https://gcc.gnu.org/viewcvs?rev=240251=gcc=rev
Log:
gcc:
2016-09-19  Bernd Edlinger  

PR c++/77434
* doc/invoke.texi: Document -Wint-in-bool-context.

c-family:
2016-09-19  Bernd Edlinger  

PR c++/77434
* c.opt (Wcond-in-bool-context): New warning.
* c-common.c (c_common_truthvalue_conversion): Warn on integer
constants in boolean context.

cp:
2016-09-19  Bernd Edlinger  

PR c++/77434
* cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here.

testsuite:
2016-09-19  Bernd Edlinger  

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

Added:
trunk/gcc/testsuite/c-c++-common/Wint-in-bool-context.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/c-family/ChangeLog
trunk/gcc/c-family/c-common.c
trunk/gcc/c-family/c.opt
trunk/gcc/cp/ChangeLog
trunk/gcc/cp/cvt.c
trunk/gcc/doc/invoke.texi
trunk/gcc/testsuite/ChangeLog

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

--- Comment #11 from Bernd Edlinger  ---
As I said, I think "<<" on signed integers is generally bogus in a
truth value context.

So I tried an experiment for such a warning:

Index: c-common.c
===
--- c-common.c  (Revision 239953)
+++ c-common.c  (Arbeitskopie)
@@ -4601,6 +4601,13 @@ c_common_truthvalue_conversion (location_t locatio
   /* These don't change whether an object is nonzero or zero.  */
   return c_common_truthvalue_conversion (location, TREE_OPERAND (expr,
0));

+case LSHIFT_EXPR:
+  if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+ && !TYPE_UNSIGNED (TREE_TYPE (expr)))
+   warning_at (EXPR_LOCATION (expr), 0,
+   "<< on signed integer in boolean context");
+  break;
+
 case LROTATE_EXPR:
 case RROTATE_EXPR:
   /* These don't change whether an object is zero or nonzero, but


And guess what: it immediately found something!

In file included from ../../gcc-trunk/gcc/cp/parser.c:24:0:
../../gcc-trunk/gcc/cp/parser.c: In function 'tree_node*
cp_parser_condition(cp_parser*)':
../../gcc-trunk/gcc/cp/cp-tree.h:4964:34: error: << on signed integer in
boolean context [-Werror]
 #define LOOKUP_ONLYCONVERTING (1 << 2)
   ~~~^
../../gcc-trunk/gcc/cp/parser.c:11175:17: note: in expansion of macro
'LOOKUP_ONLYCONVERTING'
bool flags = LOOKUP_ONLYCONVERTING;
 ^
cc1plus: all warnings being treated as errors
make[3]: *** [cp/parser.o] Error 1

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

--- Comment #10 from Bernd Edlinger  ---
(In reply to Manuel López-Ibáñez from comment #9)
> It seems to me that they are two different warnings that could be triggered
> on similar code. The one warned by the patch would also warn about:
> 
> if (a ? 1 : 2)
> 
> but not about 
> 
>gcc_assert (die_offset > 0
>  && die_offset <= (loc->dw_loc_opc == DW_OP_call2)
>   ? 0
>   : 0x);
> 
> nor:
> 
>gcc_assert (die_offset > 0
>  && die_offset <= (loc->dw_loc_opc == DW_OP_call2)
>   ? n
>   : 0x);
> 
> This is what clang emits for a similar case (for which GCC does not warn):
> 
> prog.cc:3:26: warning: operator '?:' has lower precedence than '<<'; '<<'
> will be evaluated first [-Wparentheses]
>int n = a << (a == b) ? 1 :2;
>~ ^
> prog.cc:3:26: note: place parentheses around the '<<' expression to silence
> this warning
>int n = a << (a == b) ? 1 :2;
>  ^
>()
> prog.cc:3:26: note: place parentheses around the '?:' expression to evaluate
> it first
>int n = a << (a == b) ? 1 :2;
>  ^
> (  )
> 
> 
> Perhaps a middle-ground would be something like:
> 
> warning: operator '?:' has lower precedence than '=='; '==' will be
> evaluated first and '?:' will evaluate to integers in boolean context
> [-Wparentheses]
> if (a == b ? 1 : 2)
> ~~ ^
> note: place parentheses around the '==' expression to silence this warning
> if (a == b ? 1 : 2)
> ~~
> (a == b)
> note: place parentheses around the '?:' expression to evaluate it first
> if (a == b ? 1 : 2)
>  ~
>  (b ? 1 : 2)

I think normal C code like:

int x = a == b ? 1 : 2;

should not trigger a -Wall warning.

Maybe -Wextra, but even -Wextra
does not enable everything we have.
Probably because of too much false
positives?

But the warning on << at LHS of ?:
will rarely have false positives.
I mean << makes really no sense
in a truth value context.

For instance assume a is int;
then, I can show that:

"if (a << b)" would be the same as
"if (a)", because << has undefined
behaviour on overflow.  If the
user writes "<< b" then that should
not be undefined nor redundant.
The same logic that's behind my patch.
"if (a ? 1 : 2)" evaluates to "if (1)"
and that's something that can't
be right, as an assertion that is
written with 4 lines of ?: expression,
but it's evaluated to "if (0)" by the FE.

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[manu at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

--- Comment #9 from Manuel López-Ibáñez  ---
(In reply to Eric Gallager from comment #8)
> As a user, I'd prefer warning about the missing parentheses instead of the
> boolean context thing, the missing parentheses make a lot more sense to me
> and it'd be easier for me to understand how to fix it, whereas the boolean
> context one is a bit more confusing.

It seems to me that they are two different warnings that could be triggered on
similar code. The one warned by the patch would also warn about:

if (a ? 1 : 2)

but not about 

   gcc_assert (die_offset > 0
 && die_offset <= (loc->dw_loc_opc == DW_OP_call2)
  ? 0
  : 0x);

nor:

   gcc_assert (die_offset > 0
 && die_offset <= (loc->dw_loc_opc == DW_OP_call2)
  ? n
  : 0x);

This is what clang emits for a similar case (for which GCC does not warn):

prog.cc:3:26: warning: operator '?:' has lower precedence than '<<'; '<<' will
be evaluated first [-Wparentheses]
   int n = a << (a == b) ? 1 :2;
   ~ ^
prog.cc:3:26: note: place parentheses around the '<<' expression to silence
this warning
   int n = a << (a == b) ? 1 :2;
 ^
   ()
prog.cc:3:26: note: place parentheses around the '?:' expression to evaluate it
first
   int n = a << (a == b) ? 1 :2;
 ^
(  )


Perhaps a middle-ground would be something like:

warning: operator '?:' has lower precedence than '=='; '==' will be evaluated
first and '?:' will evaluate to integers in boolean context [-Wparentheses]
if (a == b ? 1 : 2)
~~ ^
note: place parentheses around the '==' expression to silence this warning
if (a == b ? 1 : 2)
~~
(a == b)
note: place parentheses around the '?:' expression to evaluate it first
if (a == b ? 1 : 2)
 ~
 (b ? 1 : 2)

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[egall at gwmail dot gwu.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

Eric Gallager  changed:

   What|Removed |Added

 CC||egall at gwmail dot gwu.edu

--- Comment #8 from Eric Gallager  ---
As a user, I'd prefer warning about the missing parentheses instead of the
boolean context thing, the missing parentheses make a lot more sense to me and
it'd be easier for me to understand how to fix it, whereas the boolean context
one is a bit more confusing.

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

--- Comment #7 from Bernd Edlinger  ---
(In reply to jos...@codesourcery.com from comment #6)
> On Thu, 1 Sep 2016, bernd.edlinger at hotmail dot de wrote:
> 
> > +   warning_at (location, 0,
> > +   "?: expression using integer constants in boolean
> > context");
> 
> This should of course be enabled by some -W option (new or existing, and 
> enabled by -Wall in any case); warnings without an option to control them 
> should be avoided.

sure. It is only an experiment so far.  But it feels quite right.

The location info is not optimal: at the closing parenthesis.
I am not sure how to name the warning, and if there is a better
way to tell the user what's wrong with the code.

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

--- Comment #6 from joseph at codesourcery dot com  ---
On Thu, 1 Sep 2016, bernd.edlinger at hotmail dot de wrote:

> +   warning_at (location, 0,
> +   "?: expression using integer constants in boolean
> context");

This should of course be enabled by some -W option (new or existing, and 
enabled by -Wall in any case); warnings without an option to control them 
should be avoided.

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

--- Comment #5 from Bernd Edlinger  ---
This is an idea for a warning that does not focus on parentheses:

Here we had: a ? c1 : c2;
but in a context where a boolean is requested.
It is always suspicious, when c1, and c2 are integer constants
which a neither 0 or 1:

Index: gcc/c-family/c-common.c
===
--- gcc/c-family/c-common.c (revision 239944)
+++ gcc/c-family/c-common.c (working copy)
@@ -4618,6 +4618,14 @@
   TREE_OPERAND (expr, 0));

 case COND_EXPR:
+  if (TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST
+ && TREE_CODE (TREE_OPERAND (expr, 2)) == INTEGER_CST
+ && !integer_zerop (TREE_OPERAND (expr, 1))
+ && !integer_onep (TREE_OPERAND (expr, 1))
+ && !integer_zerop (TREE_OPERAND (expr, 2))
+ && !integer_onep (TREE_OPERAND (expr, 2)))
+   warning_at (location, 0,
+   "?: expression using integer constants in boolean
context");
   /* Distribute the conversion into the arms of a COND_EXPR.  */
   if (c_dialect_cxx ())
{



Bootstrap obviously fails here:
In file included from ../../gcc-trunk/gcc/dwarf2out.c:59:0:
../../gcc-trunk/gcc/dwarf2out.c: In function 'void
output_loc_operands(dw_loc_descr_ref, int)':
../../gcc-trunk/gcc/system.h:725:18: error: ?: expression using integer
constants in boolean context [-Werror]
((void)(!(EXPR) ? fancy_abort (__FILE__, __LINE__, __FUNCTION__), 0 : 0))
  ^
../../gcc-trunk/gcc/dwarf2out.c:2053:2: note: in expansion of macro
'gcc_assert'
  gcc_assert (die_offset > 0
  ^~
../../gcc-trunk/gcc/system.h:725:18: error: ?: expression using integer
constants in boolean context [-Werror]
((void)(!(EXPR) ? fancy_abort (__FILE__, __LINE__, __FUNCTION__), 0 : 0))
  ^
../../gcc-trunk/gcc/dwarf2out.c:2053:2: note: in expansion of macro
'gcc_assert'
  gcc_assert (die_offset > 0
  ^~
cc1plus: all warnings being treated as errors

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

Bernd Edlinger  changed:

   What|Removed |Added

 CC||bernd.edlinger at hotmail dot 
de

--- Comment #4 from Bernd Edlinger  ---
(In reply to jos...@codesourcery.com from comment #1)
> I think the key thing that makes this suspicious is the boolean context 
> combined with both halves of the conditional expression being constant.  
> A conditional expression with both halves constant in a boolean context 
> either always evaluates to the same value, as here, or could be replaced 
> simply by "(COND)" or "(!(COND))".
> 
> Alternatively, a conditional expression in a boolean context where either 
> half is a constant that's not 0 or 1 is suspicious.

I agree: <= for boolean seems questionable.

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[glisse at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

--- Comment #3 from Marc Glisse  ---
(In reply to jos...@codesourcery.com from comment #2)
> following is not suspicious and it would seem silly to warn for it:
> 
> return (a > 0 && b <= 3 ? 1 : 2);
> 
> (because the suggested alternative parse would involve b <= 3 ? 1 : 2 as 
> RHS of &&, which is unnatural as a conditional expression in boolean 
> context where the halves of the expression aren't boolean).

While there is only one sensible parsing, in a code review I would still ask
for parentheses, so that the reader can more quickly find that one sensible
parsing and be certain that the compiler agrees. But it becomes a matter of
style, and people complain every time we add a style warning, so maybe your
restricted warning would be best (easier to include in -Wall).

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

--- Comment #2 from joseph at codesourcery dot com  ---
On Wed, 31 Aug 2016, joseph at codesourcery dot com wrote:

> > Code such as the following are suspicious:
> > 
> > int foo(int a, int b)
> > {
> > return (a > 0 && a <= (b == 1) ? 1 : 2);
> 
> Actually I don't think this is suspicious at all; it's just an ordinary 
> conditional expression and this precedence rule doesn't tend to confuse 
> people normally.  Certainly it's less dubious than various existing cases 
> warned about for precedence.

Or, if it's suspicious, it's only because of the boolean expression (b == 
1) in the RHS of <=, where the alternative parse makes more sense.  The 
following is not suspicious and it would seem silly to warn for it:

return (a > 0 && b <= 3 ? 1 : 2);

(because the suggested alternative parse would involve b <= 3 ? 1 : 2 as 
RHS of &&, which is unnatural as a conditional expression in boolean 
context where the halves of the expression aren't boolean).

[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77434

--- Comment #1 from joseph at codesourcery dot com  ---
On Wed, 31 Aug 2016, manu at gcc dot gnu.org wrote:

> Code such as the following are suspicious:
> 
> int foo(int a, int b)
> {
> return (a > 0 && a <= (b == 1) ? 1 : 2);

Actually I don't think this is suspicious at all; it's just an ordinary 
conditional expression and this precedence rule doesn't tend to confuse 
people normally.  Certainly it's less dubious than various existing cases 
warned about for precedence.

> Real bug in GCC: PR77421
> 
> static void
> output_loc_operands (dw_loc_descr_ref loc, int for_eh_or_skip)
> {
>   unsigned long die_offset
> = get_ref_die_offset (val1->v.val_die_ref.die);
>   
>   gcc_assert (die_offset > 0
> && die_offset <= (loc->dw_loc_opc == DW_OP_call2)
>  ? 0x
>  : 0x);

I think the key thing that makes this suspicious is the boolean context 
combined with both halves of the conditional expression being constant.  
A conditional expression with both halves constant in a boolean context 
either always evaluates to the same value, as here, or could be replaced 
simply by "(COND)" or "(!(COND))".

Alternatively, a conditional expression in a boolean context where either 
half is a constant that's not 0 or 1 is suspicious.