On Wed, 16 Feb 2022 at 00:30, Zhao Wei Liew <zhaoweil...@gmail.com> wrote:
> 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 > > Sorry, it seems like this line got wrapped again. I'll try my best to fix it in future patches! > 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 > >
