On Thu, 26 Oct 2023, Patrick Palka wrote:
> On Thu, 26 Oct 2023, Jason Merrill wrote:
>
> > On 10/25/23 14:55, Patrick Palka wrote:
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
> > > OK for trunk?
> > >
> > > -- >8 --
> > >
> > > Now that we don't have to worry about looking thruogh NON_DEPENDENT_EXPR,
> > > we can easily extend the -Wparentheses warning in convert_for_assignment
> > > to consider (non-dependent) templated assignment operator expressions as
> > > well, like r14-4111-g6e92a6a2a72d3b did in maybe_convert_cond.
> > >
> > > gcc/cp/ChangeLog:
> > >
> > > * cp-tree.h (is_assignment_op_expr_p): Declare.
> > > * semantics.cc (is_assignment_op_expr_p): Generalize to return
> > > true for assignment operator expression, not just one that
> > > have been resolved to an operator overload.
> > > (maybe_convert_cond): Remove now-redundant checks around
> > > is_assignment_op_expr_p.
> > > * typeck.cc (convert_for_assignment): Look through implicit
> > > INDIRECT_REF in -Wparentheses warning logic, and generalize
> > > to use is_assignment_op_expr_p.
> >
> > Do we want to factor out the whole warning logic rather than adjust it in
> > both
> > places?
>
> Sounds good, like so? Bootstrap / regtest in progress.
>
> -- >8 --
>
> Subject: [PATCH] c++: more ahead-of-time -Wparentheses warnings
>
> Now that we don't have to worry about looking through NON_DEPENDENT_EXPR,
> we can easily extend the -Wparentheses warning in convert_for_assignment
> to consider (non-dependent) templated assignment operator expressions as
> well, like r14-4111-g6e92a6a2a72d3b did in maybe_convert_cond.
>
> gcc/cp/ChangeLog:
>
> * cp-tree.h (maybe_warn_unparenthesized_assignment): Declare.
> * semantics.cc (is_assignment_op_expr_p): Generalize to return
> true for assignment operator expression, not just one that
> have been resolved to an operator overload.
> (maybe_warn_unparenthesized_assignment): Factored out from ...
> (maybe_convert_cond): ... here.
> (finish_parenthesized_expr): Also mention
> maybe_warn_unparenthesized_assignment.
> * typeck.cc (convert_for_assignment): Replace -Wparentheses
> warning logic with maybe_warn_unparenthesized_assignment.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/warn/Wparentheses-13.C: Strengthen by expecting that
> the -Wparentheses warning are issued ahead of time.
> * g++.dg/warn/Wparentheses-23.C: Likewise.
> * g++.dg/warn/Wparentheses-32.C: Remove xfails.
> ---
> gcc/cp/cp-tree.h | 1 +
> gcc/cp/semantics.cc | 55 ++++++++++++++-------
> gcc/cp/typeck.cc | 13 ++---
> gcc/testsuite/g++.dg/warn/Wparentheses-13.C | 2 -
> gcc/testsuite/g++.dg/warn/Wparentheses-23.C | 3 --
> gcc/testsuite/g++.dg/warn/Wparentheses-32.C | 8 +--
> 6 files changed, 44 insertions(+), 38 deletions(-)
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 30fe716b109..98b29e9cf81 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7875,6 +7875,7 @@ extern tree lambda_regenerating_args (tree);
> extern tree most_general_lambda (tree);
> extern tree finish_omp_target (location_t, tree,
> tree, bool);
> extern void finish_omp_target_clauses (location_t, tree, tree
> *);
> +extern void maybe_warn_unparenthesized_assignment (tree, tsubst_flags_t);
>
> /* in tree.cc */
> extern int cp_tree_operand_length (const_tree);
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 72ec72de690..5664da9f4f2 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -840,15 +840,20 @@ finish_goto_stmt (tree destination)
> return add_stmt (build_stmt (input_location, GOTO_EXPR, destination));
> }
>
> -/* Returns true if CALL is a (possibly wrapped) CALL_EXPR or AGGR_INIT_EXPR
> - to operator= () that is written as an operator expression. */
> +/* Returns true if T corresponds to an assignment operator expression. */
> +
> static bool
> -is_assignment_op_expr_p (tree call)
> +is_assignment_op_expr_p (tree t)
> {
> - if (call == NULL_TREE)
> + if (t == NULL_TREE)
> return false;
>
> - call = extract_call_expr (call);
> + if (TREE_CODE (t) == MODIFY_EXPR
> + || (TREE_CODE (t) == MODOP_EXPR
> + && TREE_CODE (TREE_OPERAND (t, 1)) == NOP_EXPR))
> + return true;
> +
> + tree call = extract_call_expr (t);
> if (call == NULL_TREE
> || call == error_mark_node
> || !CALL_EXPR_OPERATOR_SYNTAX (call))
> @@ -860,6 +865,28 @@ is_assignment_op_expr_p (tree call)
> && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR);
> }
>
> +/* Maybe warn about an unparenthesized 'a = b' (appearing in a boolean
> + context). */
> +
> +void
> +maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain)
> +{
> + t = REFERENCE_REF_P (t) ? TREE_OPERAND (t, 0) : t;
Consider this to instead be written more idiomatically as
if (REFERENCE_REF_P (t))
t = TREE_OPERAND (t, 0);
> +
> + if ((complain & tf_warning)
> + && warn_parentheses
> + && is_assignment_op_expr_p (t)
> + /* A parenthesized expression would've gotten this warning
> + suppressed by finish_parenthesized_expr. */
> + && !warning_suppressed_p (t, OPT_Wparentheses))
> + {
> + warning_at (cp_expr_loc_or_input_loc (t),
> + OPT_Wparentheses, "suggest parentheses around "
> + "assignment used as truth value");
> + suppress_warning (t, OPT_Wparentheses);
> + }
> +}
> +
> /* COND is the condition-expression for an if, while, etc.,
> statement. Convert it to a boolean value, if appropriate.
> In addition, verify sequence points if -Wsequence-point is enabled. */
> @@ -878,21 +905,10 @@ maybe_convert_cond (tree cond)
> if (warn_sequence_point && !processing_template_decl)
> verify_sequence_points (cond);
>
> + maybe_warn_unparenthesized_assignment (cond, tf_warning_or_error);
> +
> /* Do the conversion. */
> cond = convert_from_reference (cond);
> -
> - tree inner = REFERENCE_REF_P (cond) ? TREE_OPERAND (cond, 0) : cond;
> - if ((TREE_CODE (inner) == MODIFY_EXPR
> - || (TREE_CODE (inner) == MODOP_EXPR
> - && TREE_CODE (TREE_OPERAND (inner, 1)) == NOP_EXPR)
> - || is_assignment_op_expr_p (inner))
> - && warn_parentheses
> - && !warning_suppressed_p (inner, OPT_Wparentheses)
> - && warning_at (cp_expr_loc_or_input_loc (inner),
> - OPT_Wparentheses, "suggest parentheses around "
> - "assignment used as truth value"))
> - suppress_warning (inner, OPT_Wparentheses);
> -
> return condition_conversion (cond);
> }
>
> @@ -2158,7 +2174,8 @@ finish_parenthesized_expr (cp_expr expr)
> {
> if (EXPR_P (expr))
> {
> - /* This inhibits warnings in c_common_truthvalue_conversion. */
> + /* This inhibits warnings in maybe_warn_unparenthesized_assignment
> + and c_common_truthvalue_conversion. */
> tree inner = REFERENCE_REF_P (expr) ? TREE_OPERAND (expr, 0) : *expr;
> suppress_warning (inner, OPT_Wparentheses);
> }
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 00570f170ba..49afbd8fb5e 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -10338,16 +10338,9 @@ convert_for_assignment (tree type, tree rhs,
>
> /* If -Wparentheses, warn about a = b = c when a has type bool and b
> does not. */
> - if (warn_parentheses
> - && TREE_CODE (type) == BOOLEAN_TYPE
> - && TREE_CODE (rhs) == MODIFY_EXPR
> - && !warning_suppressed_p (rhs, OPT_Wparentheses)
> - && TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE
> - && (complain & tf_warning)
> - && warning_at (rhs_loc, OPT_Wparentheses,
> - "suggest parentheses around assignment used as "
> - "truth value"))
> - suppress_warning (rhs, OPT_Wparentheses);
> + if (TREE_CODE (type) == BOOLEAN_TYPE
> + && TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
> + maybe_warn_unparenthesized_assignment (rhs, complain);
>
> if (complain & tf_warning)
> warn_for_address_or_pointer_of_packed_member (type, rhs);
> diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-13.C
> b/gcc/testsuite/g++.dg/warn/Wparentheses-13.C
> index 22a139f23a4..d6438942c28 100644
> --- a/gcc/testsuite/g++.dg/warn/Wparentheses-13.C
> +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-13.C
> @@ -65,5 +65,3 @@ bar (T)
> d = (a = a);
> foo (27);
> }
> -
> -template void bar<int> (int); // { dg-message "required" }
> diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-23.C
> b/gcc/testsuite/g++.dg/warn/Wparentheses-23.C
> index f1749c2b8da..bd7195e5023 100644
> --- a/gcc/testsuite/g++.dg/warn/Wparentheses-23.C
> +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-23.C
> @@ -114,8 +114,5 @@ bar4 (T)
> return (a = a);
> }
>
> -template void bar<int> (int); // { dg-message "required" }
> -template bool bar1<int> (int); // { dg-message "required" }
> template bool bar2<int> (int);
> -template bool bar3<int> (int); // { dg-message "required" }
> template bool bar4<int> (int);
> diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-32.C
> b/gcc/testsuite/g++.dg/warn/Wparentheses-32.C
> index 719a9d9e73a..b03515afd55 100644
> --- a/gcc/testsuite/g++.dg/warn/Wparentheses-32.C
> +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-32.C
> @@ -21,8 +21,8 @@ void f() {
> if (z1 = z2) { } // { dg-warning "parentheses" }
>
> bool b;
> - b = m = n; // { dg-warning "parentheses" "" { xfail *-*-* } }
> - b = x1 = x2; // { dg-warning "parentheses" "" { xfail *-*-* } }
> - b = y1 = y2; // { dg-warning "parentheses" "" { xfail *-*-* } }
> - b = z1 = z2; // { dg-warning "parentheses" "" { xfail *-*-* } }
> + b = m = n; // { dg-warning "parentheses" }
> + b = x1 = x2; // { dg-warning "parentheses" }
> + b = y1 = y2; // { dg-warning "parentheses" }
> + b = z1 = z2; // { dg-warning "parentheses" }
> }
> --
> 2.42.0.482.g2e8e77cbac
>
>
