On Tue, 15 Feb 2022 at 21:05, Jason Merrill <ja...@redhat.com> wrote: > >> > >> 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? > > > > I think instead of factoring out a new function, let's change the assert > > to an if and return NULL_TREE if it fails. >
I've adjusted the patch as advised. What do you think? > Incidentally, the subject should be "c++:" instead of "c:". > Ah, I see. I found it a bit odd that gcc-commit-mklog auto-generated a subject with "c:", but I just went with it as I didn't know any better. Unfortunately, I can't change it now on the current thread. > Also, it doesn't look like you have a copyright assignment with the FSF, > so you will need to add a DCO sign-off to your patches; see > https://gcc.gnu.org/dco.html for more information. > > I'd suggest putting your revision history before the scissors line, as > that doesn't need to be part of the commit message. > Got it. Made the changes in the latest patch. > And the latest patch didn't apply easily because this line: > > >> diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-31.C > >> b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C > > got wrapped in transit. > Ah, I didn't notice that. Sorry about that! I'm kinda new to the whole mailing list setup so there are some kinks I have to iron out. v5: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590393.html Changes since v5: 1. Revert changes in v4. 2. Replace gcc_assert with a return NULL_TREE in extract_call_expr. 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 (. ------ Everything below is patch v6 ------ 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. PR c++/25689 gcc/cp/ChangeLog: * call.cc (extract_call_expr): Return a NULL_TREE on failure instead of asserting. * 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. Signed-off-by: Zhao Wei Liew <zhaoweil...@gmail.com> --- gcc/cp/call.cc | 7 ++- gcc/cp/semantics.cc | 20 ++++++- gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++ 3 files changed, 85 insertions(+), 4 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 d6eed5ed835..3b2c6d8c499 100644 --- a/gcc/cp/call.cc +++ b/gcc/cp/call.cc @@ -7090,9 +7090,10 @@ extract_call_expr (tree call) default:; } - gcc_assert (TREE_CODE (call) == CALL_EXPR - || TREE_CODE (call) == AGGR_INIT_EXPR - || call == error_mark_node); + if (TREE_CODE (call) != CALL_EXPR + && TREE_CODE (call) != AGGR_INIT_EXPR + && call != error_mark_node) + return NULL_TREE; return call; } diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 0cb17a6a8ab..7a8f317af0d 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -815,6 +815,24 @@ 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 (call); + if (call == NULL_TREE || !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 +854,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 00000000000..8d48ca52057 --- /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
