Implement -Wduplicated-branches (PR c/64279) (v3)

[Jeff Law]

On 01/09/2017 02:21 AM, Marek Polacek wrote:
> On Thu, Jan 05, 2017 at 04:41:28PM +0100, Jakub Jelinek wrote:
>> On Thu, Jan 05, 2017 at 04:39:40PM +0100, Marek Polacek wrote:
>>> Coming back to this...
>>
>>>> Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0)
>>>> or so (the exact last operand needs to be figured out).
>>>> OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to mean the
>>>> same thing.  0 is a tiny bit better, but still it will give up on e.g. pure
>>>> and other calls.  OEP_PURE_SAME is tiny bit better than that, but still
>>>> calls with the same arguments to the same function will not be considered
>>>> equal, plus likely operand_equal_p doesn't handle STATEMENT_LIST etc.
>>>> So maybe we need another OEP_* mode for this.
>>>
>>> Yea, if I add "&& operand_equal_p (thenb, elseb, 0)" then this warning doesn't
>>> trigger for certain cases, such as MODIFY_EXPR, RETURN_EXPR, probably
>>> STATEMENT_LIST and others.  So I suppose I could introduce a new OEP_ mode for
>>> this (names?  OEP_EXTENDED?) and then in operand_equal_p in case tcc_expression
>>> do
>>>
>>>   case MODIFY_EXPR:
>>>     if (flags & OEP_EXTENDED)
>>>       // compare LHS and RHS of both
>>>
>>> ?
>>
>> Yeah.  Not sure what is the best name for that.  Maybe Richi has some clever
>> ideas.
>
> Here it is.  The changes in operand_equal_p should only trigger with the new
> OEP_LEXICOGRAPHIC, and given the macro location issue, the warning isn't yet
> enabled by neither -Wall nor -Wextra, so this all should be safe.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2017-01-09  Marek Polacek  <polacek@redhat.com>
>
> 	PR c/64279
> 	* c-common.h (do_warn_duplicated_branches_r): Declare.
> 	* c-gimplify.c (c_genericize): Walk the function tree calling
> 	do_warn_duplicated_branches_r.
> 	* c-warn.c (expr_from_macro_expansion_r): New.
> 	(do_warn_duplicated_branches): New.
> 	(do_warn_duplicated_branches_r): New.
> 	* c.opt (Wduplicated-branches): New option.
>
> 	* c-typeck.c (build_conditional_expr): Warn about duplicated branches.
>
> 	* call.c (build_conditional_expr_1): Warn about duplicated branches.
> 	* semantics.c (finish_expr_stmt): Build statement using the proper
> 	location.
>
> 	* doc/invoke.texi: Document -Wduplicated-branches.
> 	* fold-const.c (operand_equal_p): Handle MODIFY_EXPR, INIT_EXPR,
> 	COMPOUND_EXPR, PREDECREMENT_EXPR, PREINCREMENT_EXPR,
> 	POSTDECREMENT_EXPR, POSTINCREMENT_EXPR, CLEANUP_POINT_EXPR, EXPR_STMT,
> 	STATEMENT_LIST, and RETURN_EXPR.  For non-pure non-const functions
> 	return 0 only when not OEP_LEXICOGRAPHIC.
> 	(fold_build_cleanup_point_expr): Use the expression
> 	location when building CLEANUP_POINT_EXPR.
> 	* tree-core.h (enum operand_equal_flag): Add OEP_LEXICOGRAPHIC.
> 	* tree.c (add_expr): Handle error_mark_node.
>
> 	* c-c++-common/Wduplicated-branches-1.c: New test.
> 	* c-c++-common/Wduplicated-branches-10.c: New test.
> 	* c-c++-common/Wduplicated-branches-11.c: New test.
> 	* c-c++-common/Wduplicated-branches-12.c: New test.
> 	* c-c++-common/Wduplicated-branches-2.c: New test.
> 	* c-c++-common/Wduplicated-branches-3.c: New test.
> 	* c-c++-common/Wduplicated-branches-4.c: New test.
> 	* c-c++-common/Wduplicated-branches-5.c: New test.
> 	* c-c++-common/Wduplicated-branches-6.c: New test.
> 	* c-c++-common/Wduplicated-branches-7.c: New test.
> 	* c-c++-common/Wduplicated-branches-8.c: New test.
> 	* c-c++-common/Wduplicated-branches-9.c: New test.
> 	* c-c++-common/Wimplicit-fallthrough-7.c: Coalesce dg-warning.
> 	* g++.dg/cpp0x/lambda/lambda-switch.C: Move dg-warning.
> 	* g++.dg/ext/builtin-object-size3.C: Likewise.
> 	* g++.dg/gomp/loop-1.C: Likewise.
> 	* g++.dg/warn/Wduplicated-branches1.C: New test.
> 	* g++.dg/warn/Wduplicated-branches2.C: New test.
s/indentical/identical in the doc/invoke.texi changes.

>

> diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
> index 96e7351..ed8ffe4 100644
> --- gcc/c/c-typeck.c
> +++ gcc/c/c-typeck.c
> @@ -5193,6 +5193,15 @@ build_conditional_expr (location_t colon_loc, tree ifexp, bool ifexp_bcp,
>      ret = build1 (EXCESS_PRECISION_EXPR, semantic_result_type, ret);
>
>    protected_set_expr_location (ret, colon_loc);
> +
> +  /* If the OP1 and OP2 are the same and don't have side-effects,
> +     warn here, because the COND_EXPR will be turned into OP1.  */
> +  if (warn_duplicated_branches
> +      && TREE_CODE (ret) == COND_EXPR
> +      && (op1 == op2 || operand_equal_p (op1, op2, 0)))
Did you want OEP_LEXICOGRAPHIC here, or in this context do we not have 
to worry about the more complex forms?  What about a statement 
expressions?  Have we lowered those at this point already?

> diff --git gcc/cp/call.c gcc/cp/call.c
> index e431221..84931fb 100644
> --- gcc/cp/call.c
> +++ gcc/cp/call.c
> @@ -5302,6 +5302,13 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
>   valid_operands:
>    result = build3_loc (loc, COND_EXPR, result_type, arg1, arg2, arg3);
>
> +  /* If the ARG2 and ARG3 are the same and don't have side-effects,
> +     warn here, because the COND_EXPR will be turned into ARG2.  */
> +  if (warn_duplicated_branches
> +      && (arg2 == arg3 || operand_equal_p (arg2, arg3, 0)))
Likewise.

So, typo fix in invoke.texi and change to use OEP_LEXICOGRAPHIC in those 
two spots if needed and then OK for the trunk.

jeff