[PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Jeff Law]

On 09/15/2016 03:19 AM, Bernd Edlinger wrote:
> On 09/14/16 20:11, Jason Merrill wrote:
>>> >>
>>> >> Yes.  The reasoning I initially had was that it is completely
>>> >> pointless to have something of the form "if (x ? 1 : 2)" or
>>> >> "if (x ? 0 : 0)" because the result does not even depend on x
>>> >> in this case.  But something like "if (x ? 4999 : 0)" looks
>>> >> bogus but does at least not ignore x.
>>> >>
>>> >> If the false-positives are becoming too much of a problem here,
>>> >> then I should of course revert to the previous heuristic again.
>> >
>> > I think we could have both, where the weaker form is part of -Wall and
>> > people can explicitly select the stronger form.
>> >
>
> Yes, agreed.  So here is what I would think will be the first version.
>
> It can later be extended to cover the more pedantic cases which
> will not be enabled by -Wall.
>
> I would like to send a follow-up patch for the warning on
> signed-integer shift left in boolean context, which I think
> should also be good for Wall.
> (I already had that feature in patch version 2 but that's meanwhile
> outdated).
>
>
> Bootstrap and reg-testing on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
>
> Thanks
> Bernd.
>
>
> changelog-pr77434v3.txt
>
>
> gcc:
> 2016-09-14  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 an assertion.
>
> c-family:
> 2016-09-14  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.
>
> cp:
> 2016-09-14  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR c++/77434
> 	* cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here.
>
> testsuite:
> 2016-09-14  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR c++/77434
> 	* c-c++-common/Wcond-in-bool-context.c: New test.
>
>
> patch-pr77434v3.diff
>
>
> Index: gcc/builtins.c
> ===================================================================
> --- gcc/builtins.c	(revision 240135)
> +++ gcc/builtins.c	(working copy)
> @@ -7887,15 +7887,18 @@ fold_builtin_classify (location_t loc, tree fndecl
>  	    tree isinf_call = build_call_expr_loc (loc, isinf_fn, 1, arg);
>
>  	    signbit_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
> -					signbit_call, integer_zero_node);
> +					    signbit_call, integer_zero_node);
>  	    isinf_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
> -				      isinf_call, integer_zero_node);
> +					  isinf_call, integer_zero_node);
>
> -	    tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node, signbit_call,
> -			       integer_minus_one_node, integer_one_node);
>  	    tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
> -			       isinf_call, tmp,
> -			       integer_zero_node);
> +				   signbit_call, integer_minus_one_node,
> +				   integer_one_node);
> +	    /* Avoid a possible -Wint-in-bool-context warning in C.  */
> +	    tmp = fold_build2_loc (loc, PLUS_EXPR, integer_type_node, tmp,
> +				   integer_zero_node);
> +	    tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
> +				   isinf_call, tmp, integer_zero_node);
>  	  }
>
>  	return tmp;
This hunk is not mentioned in the ChangeLog and there's a good chance 
this has or is going to change significantly.  I don't like the tmp+0 
workaround either.  If there isn't an immediate need, can we let this 
hunk slide and come back to it after the other changes from Tamar & 
Wilco are wrapped up?

I think this is OK with the builtins.c hunk dropped as long as exclusion 
of that hunk doesn't trigger spurious warnings.


jeff