| Message ID | AM4PR0701MB21624AB3DECA1269E4BBB475E4E50@AM4PR0701MB2162.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On 09/02/2016 12:53 PM, Bernd Edlinger wrote: > Hi! > > As reported in PR77434 and PR77421 there should be a warning for > suspicious uses of conditional expressions with non-boolean arguments. > > This warning triggers on conditional expressions in boolean context, > when both possible results are non-zero integer constants, so that > the resulting truth value does in fact not depend on the condition > itself. Thus something like "if (a == b ? 1 : 2)" is always bogus, > and was most likely meant to be "if (a == (b ? 1 : 2))". > > > Boot-strap and reg-testing on x86_64-pc-linux-gnu without regressions. > Is it OK for trunk. > > > Thanks > Bernd. > > > changelog-pr77434.txt > > > gcc: > 2016-09-02 Bernd Edlinger <bernd.edlinger@hotmail.de> > > PR c++/77434 > * doc/invoke.texi: Document -Wcond-in-bool-context. > > PR middle-end/77421 > * dwarf2out.c (output_loc_operands): Fix assertion. > > c-family: > 2016-09-02 Bernd Edlinger <bernd.edlinger@hotmail.de> > > 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. > > testsuite: > 2016-09-02 Bernd Edlinger <bernd.edlinger@hotmail.de> > > PR c++/77434 > * c-c++-common/Wcond-in-bool-context.c: New test. For some reason the non-symmerty of the changes to c_common_truthvalue_conversion caused me to have to think far more about this than I probably should have. Couldn't we have a new function integer_zerop_or_onep Then use && (!integer_zerop_or_onep (TREE_OPERAND (expr, 1)) || !integer_zerop_or_onep (TREE_OPERAND (expr, 2))) Ie, if they're both constants and either is not [0,1], then we warn. With that cleanup, this is OK. jeff
On 09/12/16 21:30, Jeff Law wrote: > On 09/02/2016 12:53 PM, Bernd Edlinger wrote: >> Hi! >> >> As reported in PR77434 and PR77421 there should be a warning for >> suspicious uses of conditional expressions with non-boolean arguments. >> >> This warning triggers on conditional expressions in boolean context, >> when both possible results are non-zero integer constants, so that >> the resulting truth value does in fact not depend on the condition >> itself. Thus something like "if (a == b ? 1 : 2)" is always bogus, >> and was most likely meant to be "if (a == (b ? 1 : 2))". >> >> >> Boot-strap and reg-testing on x86_64-pc-linux-gnu without regressions. >> Is it OK for trunk. >> >> >> Thanks >> Bernd. >> >> >> changelog-pr77434.txt >> >> >> gcc: >> 2016-09-02 Bernd Edlinger <bernd.edlinger@hotmail.de> >> >> PR c++/77434 >> * doc/invoke.texi: Document -Wcond-in-bool-context. >> >> PR middle-end/77421 >> * dwarf2out.c (output_loc_operands): Fix assertion. >> >> c-family: >> 2016-09-02 Bernd Edlinger <bernd.edlinger@hotmail.de> >> >> 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. >> >> testsuite: >> 2016-09-02 Bernd Edlinger <bernd.edlinger@hotmail.de> >> >> PR c++/77434 >> * c-c++-common/Wcond-in-bool-context.c: New test. > For some reason the non-symmerty of the changes to > c_common_truthvalue_conversion caused me to have to think far more about > this than I probably should have. > > Couldn't we have a new function > integer_zerop_or_onep > > Then use > > && (!integer_zerop_or_onep (TREE_OPERAND (expr, 1)) > || !integer_zerop_or_onep (TREE_OPERAND (expr, 2))) > > Ie, if they're both constants and either is not [0,1], then we warn. > > With that cleanup, this is OK. > > jeff > Yes, I will update the patch accordingly. I agree, a statement like "if (x ? 0 : 2)" can be called suspicious as well, even if the result of x is not ignored as in "if (x ? 1 : 2)". After I posted this patch, I extended the patch to cover also suspicious int shift-left operations, at https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00155.html [PATCHv2] Add a warning for suspicious use of conditional expressions in boolean context Therefore, I would like to rename the warning to -Wint-in-bool-context so that the other patch can extend that instead of creating even more warning names with much longer names. Is renaming the warning OK as well, or could you have a look at the follow-up patch please? Thanks Bernd.
On 09/12/2016 02:00 PM, Bernd Edlinger wrote: > Yes, I will update the patch accordingly. > > I agree, a statement like "if (x ? 0 : 2)" can be called suspicious as > well, even if the result of x is not ignored as in "if (x ? 1 : 2)". > > After I posted this patch, I extended the patch to cover also > suspicious int shift-left operations, at > https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00155.html > [PATCHv2] Add a warning for suspicious use of conditional expressions in > boolean context > > Therefore, I would like to rename the warning to -Wint-in-bool-context > so that the other patch can extend that instead of creating even more > warning names with much longer names. > > > Is renaming the warning OK as well, or could you have a look at the > follow-up patch please? Renaming is OK as well. jeff
Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 239953) +++ gcc/c-family/c-common.c (working copy) @@ -4618,6 +4618,14 @@ c_common_truthvalue_conversion (location_t locatio 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_zerop (TREE_OPERAND (expr, 2)) + && (!integer_onep (TREE_OPERAND (expr, 1)) + || !integer_onep (TREE_OPERAND (expr, 2)))) + warning_at (EXPR_LOCATION (expr), OPT_Wcond_in_bool_context, + "?: using integer constants in boolean context"); /* Distribute the conversion into the arms of a COND_EXPR. */ if (c_dialect_cxx ()) { Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 239953) +++ gcc/c-family/c.opt (working copy) @@ -350,6 +350,10 @@ Wcomments C ObjC C++ ObjC++ Warning Alias(Wcomment) Synonym for -Wcomment. +Wcond-in-bool-context +C ObjC C++ ObjC++ Var(warn_cond_in_bool_context) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) +Warn for conditional expressions (?:) using integer constants in boolean context. + Wconditionally-supported C++ ObjC++ Var(warn_conditionally_supported) Warning Warn for conditionally-supported constructs. Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 239953) +++ gcc/doc/invoke.texi (working copy) @@ -259,7 +259,8 @@ Objective-C and Objective-C++ Dialects}. -Wno-attributes -Wbool-compare -Wno-builtin-macro-redefined @gol -Wc90-c99-compat -Wc99-c11-compat @gol -Wc++-compat -Wc++11-compat -Wc++14-compat -Wcast-align -Wcast-qual @gol --Wchar-subscripts -Wclobbered -Wcomment -Wconditionally-supported @gol +-Wchar-subscripts -Wclobbered -Wcomment @gol +-Wcond-in-bool-context -Wconditionally-supported @gol -Wconversion -Wcoverage-mismatch -Wno-cpp -Wdangling-else -Wdate-time @gol -Wdelete-incomplete @gol -Wno-deprecated -Wno-deprecated-declarations -Wno-designated-init @gol @@ -5179,6 +5180,13 @@ programs. Warn for variables that might be changed by @code{longjmp} or @code{vfork}. This warning is also enabled by @option{-Wextra}. +@item -Wcond-in-bool-context +@opindex Wcond-in-bool-context +@opindex Wno-cond-in-bool-context +Warn for conditional expressions (?:) using non-boolean integer constants in +boolean context, like @code{if (a <= b ? 2 : 3)}. This warning is enabled +by @option{-Wall}. + @item -Wconditionally-supported @r{(C++ and Objective-C++ only)} @opindex Wconditionally-supported @opindex Wno-conditionally-supported Index: gcc/dwarf2out.c =================================================================== --- gcc/dwarf2out.c (revision 239953) +++ gcc/dwarf2out.c (working copy) @@ -2051,9 +2051,9 @@ output_loc_operands (dw_loc_descr_ref loc, int for /* Make sure the offset has been computed and that we can encode it as an operand. */ gcc_assert (die_offset > 0 - && die_offset <= (loc->dw_loc_opc == DW_OP_call2) + && die_offset <= (loc->dw_loc_opc == DW_OP_call2 ? 0xffff - : 0xffffffff); + : 0xffffffff)); dw2_asm_output_data ((loc->dw_loc_opc == DW_OP_call2) ? 2 : 4, die_offset, NULL); } Index: gcc/testsuite/c-c++-common/Wcond-in-bool-context.c =================================================================== --- gcc/testsuite/c-c++-common/Wcond-in-bool-context.c (revision 0) +++ gcc/testsuite/c-c++-common/Wcond-in-bool-context.c (working copy) @@ -0,0 +1,17 @@ +/* PR c++/77434 */ +/* { dg-options "-Wcond-in-bool-context" } */ +/* { dg-do compile } */ + +int foo (int a, int b) +{ + if (a > 0 && a <= (b == 1) ? 1 : 2) /* { dg-warning "using integer constants in boolean context" } */ + return 1; + + if (a > 0 && a <= (b == 2) ? 1 : 1) /* { dg-bogus "using integer constants in boolean context" } */ + return 2; + + if (a > 0 && a <= (b == 3) ? 0 : 2) /* { dg-bogus "using integer constants in boolean context" } */ + return 3; + + return 0; +}