[Bug c++/77434] warn about suspicious precedence of ternary operator (?:)
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 (?:)
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 EdlingerPR 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 (?:)
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 (?:)
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 (?:)
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 (?:)
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 (?:)
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 (?:)
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 (?:)
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 (?:)
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 (?:)
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 (?:)
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 (?:)
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.