On Tue, 15 Feb 2022 at 13:13, Jason Merrill <ja...@redhat.com> wrote: > > On 2/14/22 21:30, Zhao Wei Liew wrote: > > On 14/02/2022, Jason Merrill <ja...@redhat.com> wrote: > >>> > >>> +/* Returns true if EXPR is a reference to an implicit > >>> + call to operator=(). */ > >>> +static bool > >>> +is_assignment_overload_ref_p (tree expr) > >>> +{ > >>> + if (expr == NULL_TREE || !REFERENCE_REF_P (expr)) > >>> + return false; > >> > >> This will only warn about op= that returns a reference, which is not > >> required. > >> > > > > Ah I understand now. I added some new test cases for non-reference return > > types > > and copied some code from extract_call_expr that seems to do what we want. > > Good plan, but let's call extract_call_expr rather than copy from it. >
I agree. However, I can't seem to call extract_call_expr directly because it calls gcc_assert to assert that the resulting expression is a CALL_EXPR or AGGR_INIT_EXPR. Instead, I've extracted the non-assert-related code into a extract_call_expr_noassert function and called that instead in the new patch. Is that okay? > > --- a/gcc/cp/semantics.cc > > +++ b/gcc/cp/semantics.cc > > @@ -815,6 +815,33 @@ 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 > > + to operator=() that is written as an operator expression. */ > > +static bool > > +is_assignment_op_expr_p(tree call) > > Missing space before ( > Thanks the catch. I've fixed that in the latest patch. As always, thanks so much for your feedback! ------Everything below is v5 of the patch------ When compiling the following code with g++ -Wparentheses, GCC does not warn on the if statement. For example, there is no warning for this code: struct A { A& operator=(int); operator bool(); }; void f(A a) { if (a = 0); // no warning } This is because a = 0 is a call to operator=, which GCC does not handle. This patch fixes this issue by handling calls to operator= when deciding to warn. Bootstrapped and tested on x86_64-unknown-linux-gnu. v4: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590379.html Changes since v4: 1. Refactor the non-assert-related code out of extract_call_expr and call that function instead to check for call expressions. v3: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590310.html Changes since v3: 1. Also handle COMPOUND_EXPRs and TARGET_EXPRs. v2: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590236.html Changes since v2: 1. Add more test cases in Wparentheses-31.C. 2. Refactor added logic to a function (is_assignment_overload_ref_p). 3. Use REFERENCE_REF_P instead of INDIRECT_REF_P. v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590158.html Changes since v1: 1. Use CALL_EXPR_OPERATOR_SYNTAX to avoid warnings for explicit operator=() calls. 2. Use INDIRECT_REF_P to filter implicit operator=() calls. 3. Use cp_get_callee_fndecl_nofold. 4. Add spaces before (. PR c/25689 gcc/cp/ChangeLog: * call.cc (extract_call_expr): Refactor non-assert code to extract_call_expr_noassert. (extract_call_expr_noassert): Refactor non-assert code. * cp-tree.h (extract_call_expr_noassert): Add prototype. * semantics.cc (is_assignment_op_expr_p): Add function to check if an expression is a call to an op= operator expression. (maybe_convert_cond): Handle the case of a op= operator expression for the -Wparentheses diagnostic. gcc/testsuite/ChangeLog: * g++.dg/warn/Wparentheses-31.C: New test. --- gcc/cp/call.cc | 11 +++- gcc/cp/cp-tree.h | 1 + gcc/cp/semantics.cc | 22 +++++++- gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++ 4 files changed, 94 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc index d6eed5ed83510..bead9d6b624c5 100644 --- a/gcc/cp/call.cc +++ b/gcc/cp/call.cc @@ -7059,7 +7059,7 @@ build_op_subscript (const op_location_t &loc, tree obj, convert_from_reference. */ tree -extract_call_expr (tree call) +extract_call_expr_noassert (tree call) { while (TREE_CODE (call) == COMPOUND_EXPR) call = TREE_OPERAND (call, 1); @@ -7090,6 +7090,15 @@ extract_call_expr (tree call) default:; } + return call; +} + +/* Like extract_call_expr_noassert, but also asserts that + the extracted expression is indeed a call expression. */ +tree +extract_call_expr (tree call) +{ + call = extract_call_expr_noassert (call); gcc_assert (TREE_CODE (call) == CALL_EXPR || TREE_CODE (call) == AGGR_INIT_EXPR || call == error_mark_node); diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index f253b32c3f21d..19a7a9d2c9334 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -6519,6 +6519,7 @@ extern bool null_ptr_cst_p (tree); extern bool null_member_pointer_value_p (tree); extern bool sufficient_parms_p (const_tree); extern tree type_decays_to (tree); +extern tree extract_call_expr_noassert (tree); extern tree extract_call_expr (tree); extern tree build_trivial_dtor_call (tree, bool = false); extern bool ref_conv_binds_directly_p (tree, tree); diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 0cb17a6a8ab1c..008e554273aa3 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -815,6 +815,26 @@ 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. */ +static bool +is_assignment_op_expr_p (tree call) +{ + if (call == NULL_TREE) + return false; + + call = extract_call_expr_noassert (call); + if (call == NULL_TREE + || (TREE_CODE (call) != CALL_EXPR && TREE_CODE (call) != AGGR_INIT_EXPR) + || !CALL_EXPR_OPERATOR_SYNTAX (call)) + return false; + + tree fndecl = cp_get_callee_fndecl_nofold (call); + return fndecl != NULL_TREE + && DECL_ASSIGNMENT_OPERATOR_P (fndecl) + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); +} + /* 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. */ @@ -836,7 +856,7 @@ maybe_convert_cond (tree cond) /* Do the conversion. */ cond = convert_from_reference (cond); - if (TREE_CODE (cond) == MODIFY_EXPR + if ((TREE_CODE (cond) == MODIFY_EXPR || is_assignment_op_expr_p (cond)) && warn_parentheses && !warning_suppressed_p (cond, OPT_Wparentheses) && warning_at (cp_expr_loc_or_input_loc (cond), diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-31.C b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C new file mode 100644 index 0000000000000..8d48ca5205782 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C @@ -0,0 +1,62 @@ +/* Test that -Wparentheses warns for struct/class assignments, + except for explicit calls to operator= (). */ +/* PR c/25689 */ +/* { dg-options "-Wparentheses" } */ + +struct A +{ + A& operator= (int); + A operator= (double); + operator bool (); +}; + +struct B +{ + bool x; + B& operator= (int); + B operator= (double); + operator bool (); +}; + +struct C +{ + C& operator= (int); + virtual C operator= (double); + operator bool (); +}; + +/* Test empty class */ +void f1 (A a1, A a2) +{ + if (a1 = 0); /* { dg-warning "suggest parentheses" } */ + if (a1 = 0.); /* { dg-warning "suggest parentheses" } */ + if (a1.operator= (0)); + if (a1.operator= (a2)); + + /* Ideally, we'd warn for empty classes using trivial operator= (below), + but we don't do so yet as it is a non-trivial COMPOUND_EXPR. */ + // if (a1 = a2); +} + +/* Test non-empty class */ +void f2(B b1, B b2) +{ + if (b1 = 0); /* { dg-warning "suggest parentheses" } */ + if (b1 = 0.); /* { dg-warning "suggest parentheses" } */ + if (b1 = b2); /* { dg-warning "suggest parentheses" } */ + if (b1.operator= (0)); + + /* Ideally, we wouldn't warn for non-empty classes using trivial + operator= (below), but we currently do as it is a MODIFY_EXPR. */ + // if (b1.operator= (b2)); +} + +/* Test class with vtable */ +void f3(C c1, C c2) +{ + if (c1 = 0); /* { dg-warning "suggest parentheses" } */ + if (c1 = 0.); /* { dg-warning "suggest parentheses" } */ + if (c1 = c2); /* { dg-warning "suggest parentheses" } */ + if (c1.operator= (0)); + if (c1.operator= (c2)); +} -- 2.35.1