Message ID | CALHvHFXuifhA7AwmsEga1oy9atV8HJA=eKkbqK8TXcaVE8SKqg@mail.gmail.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E6CA33858C1F for <patchwork@sourceware.org>; Thu, 10 Feb 2022 02:19:21 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E6CA33858C1F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1644459561; bh=rZCPOfk+p+4DWy1TwmTRHcRLZeTWaUNzxrCpovU+L2Y=; h=Date:Subject:To:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=lg/eP07cW3AyQlmph00EfvpSg2eIAL5p6D1OjStmcPCgn4V/Ol4F+ghkWR8DZC9zv 9JiGjQUYQemnnSWkuaA34ncW1vMQyYQq+92dSwSolLAI+tS0VQMf85TFb7Fk95HoSk bl4l1S4eMLS0GwLSzCC0cm/AHUnVZMrt8fk6Upg0= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-io1-xd2f.google.com (mail-io1-xd2f.google.com [IPv6:2607:f8b0:4864:20::d2f]) by sourceware.org (Postfix) with ESMTPS id C44973858D1E for <gcc-patches@gcc.gnu.org>; Thu, 10 Feb 2022 02:18:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C44973858D1E Received: by mail-io1-xd2f.google.com with SMTP id s18so5582922ioa.12 for <gcc-patches@gcc.gnu.org>; Wed, 09 Feb 2022 18:18:50 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=rZCPOfk+p+4DWy1TwmTRHcRLZeTWaUNzxrCpovU+L2Y=; b=2XSRxuzjTiVQUkSd2SHtYi6qT0yTy0GlGS2yRD0Am2Sv6BOwSG30bISim28mC6n5Vr g95lh3LM29JwIV+HywD9XXI6Lfox9ICGwHqPlYED+DEDBWPUEXhAPOko+iyHjZqB9Q9U oNdgwleZpPi80wL1O/XlFAwox9Zls7hQAQqVPW38VtIVbmMqr/cWNo0M2S97OSool7FA rq4AW7q7IU/PZvWl0NeMA0u8sQdmzxY8hJb+MTm3bBraxQqzUB62Rbo/qkOR7xkWJuBw Uw3HOtsRDTW4ZoNFD8zsKdvi+R4yTFzhNOgH6rn9Gu1VyAmgFdA30P/cLxrIH+h4orzw 08Dw== X-Gm-Message-State: AOAM533ZM0LByYJOvTNEMek5H3e8n1qFenUuIzRT/7NpJsMGMYJP9X1I PEOJQG++MzJsjXp6rxQ0vxgRBWuTU5tR03ZPKj+A8xpK3Fk= X-Google-Smtp-Source: ABdhPJxL5KhrGNcXEGMzrdz0hWK6/ORHmDh9HE7t4hnTaZzwKcQ/owupahCeCAEtVi9SVVQ2Cczcs/LNHHpANq85UsY= X-Received: by 2002:a05:6638:1652:: with SMTP id a18mr2686223jat.201.1644459528781; Wed, 09 Feb 2022 18:18:48 -0800 (PST) MIME-Version: 1.0 Received: by 2002:a05:6e02:1584:0:0:0:0 with HTTP; Wed, 9 Feb 2022 18:18:48 -0800 (PST) Date: Thu, 10 Feb 2022 10:18:48 +0800 Message-ID: <CALHvHFXuifhA7AwmsEga1oy9atV8HJA=eKkbqK8TXcaVE8SKqg@mail.gmail.com> Subject: [PATCH] c: Add diagnostic when operator= is used as truth cond [PR25689] To: GCC Patches <gcc-patches@gcc.gnu.org> Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Zhao Wei Liew via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Zhao Wei Liew <zhaoweiliew@gmail.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
c: Add diagnostic when operator= is used as truth cond [PR25689]
|
|
Commit Message
Zhao Wei Liew
Feb. 10, 2022, 2:18 a.m. UTC
Hi! I wrote a patch for PR 25689, but I feel like it may not be the ideal fix. Furthermore, there are some standing issues with the patch for which I would like tips on how to fix them. Specifically, there are 2 issues: 1. GCC warns about if (a.operator=(0)). That said, this may not be a major issue as I don't think such code is widely written. 2. GCC does not warn for `if (a = b)` where the default copy/move assignment operator is used. I've included a code snippet in PR25689 that shows the 2 issues I mentioned. I appreciate any feedback, thanks! ----Everything below is the actual patch---- When compiling the following code with g++ -Wparentheses, GCC does not warn on the if statement: 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 check for. This patch fixes that by checking for calls to operator= when deciding to warn. PR c/25689 gcc/cp/ChangeLog: * semantics.cc (maybe_convert_cond): Handle the operator=() case as well. gcc/testsuite/ChangeLog: * g++.dg/warn/Wparentheses-31.C: New test. --- gcc/cp/semantics.cc | 14 +++++++++++++- gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 11 +++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C -- 2.17.1
Comments
On 2/9/22 21:18, Zhao Wei Liew via Gcc-patches wrote: > Hi! > > I wrote a patch for PR 25689, but I feel like it may not be the ideal > fix. Furthermore, there are some standing issues with the patch for > which I would like tips on how to fix them. > Specifically, there are 2 issues: > 1. GCC warns about if (a.operator=(0)). That said, this may not be a > major issue as I don't think such code is widely written. Can you avoid this by checking CALL_EXPR_OPERATOR_SYNTAX? > 2. GCC does not warn for `if (a = b)` where the default copy/move > assignment operator is used. The code for trivial copy-assignment should be pretty recognizable, as a MODIFY_EXPR of two MEM_REFs; it's built in build_over_call after the comment "We must only copy the non-tail padding parts." > I've included a code snippet in PR25689 that shows the 2 issues I > mentioned. I appreciate any feedback, thanks! > > ----Everything below is the actual patch---- > > When compiling the following code with g++ -Wparentheses, GCC does not > warn on the if statement: > > 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 check > for. > > This patch fixes that by checking for calls to operator= when deciding > to warn. > > PR c/25689 > > gcc/cp/ChangeLog: > > * semantics.cc (maybe_convert_cond): Handle the operator=() case > as well. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wparentheses-31.C: New test. > --- > gcc/cp/semantics.cc | 14 +++++++++++++- > gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 11 +++++++++++ > 2 files changed, 24 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index 466d6b56871f4..6a25d039585f2 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -836,7 +836,19 @@ maybe_convert_cond (tree cond) > /* Do the conversion. */ > cond = convert_from_reference (cond); > > - if (TREE_CODE (cond) == MODIFY_EXPR > + /* Also check if this is a call to operator=(). > + Example: if (my_struct = 5) {...} > + */ > + tree fndecl = NULL_TREE; > + if (TREE_OPERAND_LENGTH(cond) >= 1) { > + fndecl = cp_get_callee_fndecl(TREE_OPERAND(cond, 0)); Let's use cp_get_callee_fndecl_nofold. Please add a space before all ( > + } > + > + if ((TREE_CODE (cond) == MODIFY_EXPR > + || (fndecl != NULL_TREE > + && DECL_OVERLOADED_OPERATOR_P(fndecl) > + && DECL_OVERLOADED_OPERATOR_IS(fndecl, NOP_EXPR) > + && DECL_ASSIGNMENT_OPERATOR_P(fndecl))) > && 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..abd7476ccb461 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C > @@ -0,0 +1,11 @@ > +/* PR c/25689 */ > +/* { dg-options "-Wparentheses" } */ > + > +struct A { > + A& operator=(int); > + operator bool(); > +}; > + > +void f(A a) { > + if (a = 0); /* { dg-warning "suggest parentheses" } */ > +} > -- > 2.17.1 >
On Fri, 11 Feb 2022 at 00:14, Jason Merrill <jason@redhat.com> wrote: > > On 2/9/22 21:18, Zhao Wei Liew via Gcc-patches wrote: > > Hi! > > > > I wrote a patch for PR 25689, but I feel like it may not be the ideal > > fix. Furthermore, there are some standing issues with the patch for > > which I would like tips on how to fix them. > > Specifically, there are 2 issues: > > 1. GCC warns about if (a.operator=(0)). That said, this may not be a > > major issue as I don't think such code is widely written. > > Can you avoid this by checking CALL_EXPR_OPERATOR_SYNTAX? Thanks! It worked. There is no longer a warning for `if (a.operator=(0))`. The updated patch is at the bottom of this email. > > > 2. GCC does not warn for `if (a = b)` where the default copy/move > > assignment operator is used. > > The code for trivial copy-assignment should be pretty recognizable, as a > MODIFY_EXPR of two MEM_REFs; it's built in build_over_call after the > comment "We must only copy the non-tail padding parts." Ah, I see what you mean. Thanks! However, it seems like that's the case only for non-empty classes. GCC already warns for MODIFY_EXPR, so there's nothing we need to do there. On the other hand, for empty classes, it seems that a COMPOUND_EXPR is built in build_over_call under the is_really_empty_class guard (line 9791). I don't understand the tree structure that I should identify though. Could you give me any further explanations on that? > > - if (TREE_CODE (cond) == MODIFY_EXPR > > + /* Also check if this is a call to operator=(). > > + Example: if (my_struct = 5) {...} > > + */ > > + tree fndecl = NULL_TREE; > > + if (TREE_OPERAND_LENGTH(cond) >= 1) { > > + fndecl = cp_get_callee_fndecl(TREE_OPERAND(cond, 0)); > > Let's use cp_get_callee_fndecl_nofold. > > Please add a space before all ( Got it. May I know why it's better to use *_nofold here? On an unrelated note, I adjusted the if condition to use INDIRECT_REF_P (cond) instead of TREE_OPERAND_LENGTH (cond) >= 1. I hope that's better for semantics. ------Everything below is the updated patch----- When compiling the following code with g++ -Wparentheses, GCC does not warn on the if statement: 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 check for. This patch fixes that by checking for calls to operator= when deciding to warn. v1: 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: * semantics.cc (maybe_convert_cond): Handle the operator=() case as well. gcc/testsuite/ChangeLog: * g++.dg/warn/Wparentheses-31.C: New test. --- gcc/cp/semantics.cc | 18 +++++++++++++++++- gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 11 +++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 466d6b56871f4..b45903a6a6fde 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -836,7 +836,23 @@ maybe_convert_cond (tree cond) /* Do the conversion. */ cond = convert_from_reference (cond); - if (TREE_CODE (cond) == MODIFY_EXPR + /* Check for operator syntax calls to operator=(). + Example: if (my_struct = 5) {...} + */ + tree fndecl = NULL_TREE; + if (INDIRECT_REF_P (cond)) { + tree fn = TREE_OPERAND (cond, 0); + if (TREE_CODE (fn) == CALL_EXPR + && CALL_EXPR_OPERATOR_SYNTAX (fn)) { + fndecl = cp_get_callee_fndecl_nofold (fn); + } + } + + if ((TREE_CODE (cond) == MODIFY_EXPR + || (fndecl != NULL_TREE + && DECL_OVERLOADED_OPERATOR_P (fndecl) + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR) + && DECL_ASSIGNMENT_OPERATOR_P (fndecl))) && 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..abd7476ccb461 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C @@ -0,0 +1,11 @@ +/* PR c/25689 */ +/* { dg-options "-Wparentheses" } */ + +struct A { + A& operator=(int); + operator bool(); +}; + +void f(A a) { + if (a = 0); /* { dg-warning "suggest parentheses" } */ +} -- 2.17.1
On 2/10/22 23:01, Zhao Wei Liew wrote: > On Fri, 11 Feb 2022 at 00:14, Jason Merrill <jason@redhat.com> wrote: >> >> On 2/9/22 21:18, Zhao Wei Liew via Gcc-patches wrote: >>> Hi! >>> >>> I wrote a patch for PR 25689, but I feel like it may not be the ideal >>> fix. Furthermore, there are some standing issues with the patch for >>> which I would like tips on how to fix them. >>> Specifically, there are 2 issues: >>> 1. GCC warns about if (a.operator=(0)). That said, this may not be a >>> major issue as I don't think such code is widely written. >> >> Can you avoid this by checking CALL_EXPR_OPERATOR_SYNTAX? > > Thanks! It worked. There is no longer a warning for `if (a.operator=(0))`. > The updated patch is at the bottom of this email. > >> >>> 2. GCC does not warn for `if (a = b)` where the default copy/move >>> assignment operator is used. >> >> The code for trivial copy-assignment should be pretty recognizable, as a >> MODIFY_EXPR of two MEM_REFs; it's built in build_over_call after the >> comment "We must only copy the non-tail padding parts." > > Ah, I see what you mean. Thanks! However, it seems like that's the case only > for non-empty classes. GCC already warns for MODIFY_EXPR, so there's > nothing we need to do there. > > On the other hand, for empty classes, it seems that a COMPOUND_EXPR > is built in build_over_call under the is_really_empty_class guard (line 9791). > I don't understand the tree structure that I should identify though. > Could you give me any further explanations on that? True, that's not very recognizable. I suppose we could use a TREE_LANG_FLAG to mark that COMPOUND_EXPR, or we could leave empty classes alone; neither assignment nor comparison of empty classes should happen much in practice. >>> - if (TREE_CODE (cond) == MODIFY_EXPR >>> + /* Also check if this is a call to operator=(). >>> + Example: if (my_struct = 5) {...} >>> + */ >>> + tree fndecl = NULL_TREE; >>> + if (TREE_OPERAND_LENGTH(cond) >= 1) { >>> + fndecl = cp_get_callee_fndecl(TREE_OPERAND(cond, 0)); >> >> Let's use cp_get_callee_fndecl_nofold. >> >> Please add a space before all ( > > Got it. May I know why it's better to use *_nofold here? To avoid trying to do constexpr evaluation of the function operand when it's non-constant; in the interesting cases it won't be needed. However, have you tested virtual operator=? > On an unrelated note, I adjusted the if condition to use INDIRECT_REF_P (cond) > instead of TREE_OPERAND_LENGTH (cond) >= 1. > I hope that's better for semantics. Yes; REFERENCE_REF_P might be even better. > ------Everything below is the updated patch----- > > When compiling the following code with g++ -Wparentheses, GCC does not > warn on the if statement: > > 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 check > for. > > This patch fixes that by checking for calls to operator= when deciding > to warn. > > v1: 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: > > * semantics.cc (maybe_convert_cond): Handle the operator=() case > as well. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wparentheses-31.C: New test. > --- > gcc/cp/semantics.cc | 18 +++++++++++++++++- > gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 11 +++++++++++ > 2 files changed, 28 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index 466d6b56871f4..b45903a6a6fde 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -836,7 +836,23 @@ maybe_convert_cond (tree cond) > /* Do the conversion. */ > cond = convert_from_reference (cond); > > - if (TREE_CODE (cond) == MODIFY_EXPR > + /* Check for operator syntax calls to operator=(). > + Example: if (my_struct = 5) {...} > + */ > + tree fndecl = NULL_TREE; > + if (INDIRECT_REF_P (cond)) { The { should go on the next line. > + tree fn = TREE_OPERAND (cond, 0); > + if (TREE_CODE (fn) == CALL_EXPR > + && CALL_EXPR_OPERATOR_SYNTAX (fn)) { > + fndecl = cp_get_callee_fndecl_nofold (fn); > + } > + } > + > + if ((TREE_CODE (cond) == MODIFY_EXPR > + || (fndecl != NULL_TREE > + && DECL_OVERLOADED_OPERATOR_P (fndecl) > + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR) > + && DECL_ASSIGNMENT_OPERATOR_P (fndecl))) > && 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..abd7476ccb461 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C > @@ -0,0 +1,11 @@ > +/* PR c/25689 */ > +/* { dg-options "-Wparentheses" } */ > + > +struct A { > + A& operator=(int); > + operator bool(); > +}; > + > +void f(A a) { > + if (a = 0); /* { dg-warning "suggest parentheses" } */ > +} > -- > 2.17.1 >
On Fri, 11 Feb 2022 at 20:47, Jason Merrill <jason@redhat.com> wrote: > > > > On the other hand, for empty classes, it seems that a COMPOUND_EXPR > > is built in build_over_call under the is_really_empty_class guard (line 9791). > > I don't understand the tree structure that I should identify though. > > Could you give me any further explanations on that? > > True, that's not very recognizable. I suppose we could use a > TREE_LANG_FLAG to mark that COMPOUND_EXPR, or we could leave empty > classes alone; neither assignment nor comparison of empty classes should > happen much in practice. I agree. I'll leave them alone. > > > > Got it. May I know why it's better to use *_nofold here? > > To avoid trying to do constexpr evaluation of the function operand when > it's non-constant; in the interesting cases it won't be needed. > > However, have you tested virtual operator=? > Yup, everything seems to work as expected for structs using virtual operator=. I've updated the test suite to reflect the tests. One thing to note: I've commented out 2 test statements that shouldn't work. One of them is caused by trivial assignments in empty classes being COMPOUND_EXPRs as we discussed above. The other is an existing issue caused by trivial assignments in non-empty classes being MODIFY_EXPRs. > > On an unrelated note, I adjusted the if condition to use INDIRECT_REF_P (cond) > > instead of TREE_OPERAND_LENGTH (cond) >= 1. > > I hope that's better for semantics. > > Yes; REFERENCE_REF_P might be even better. > Alright, I've changed it to REFERENCE_REF_P and it seems to work fine as well. -----Everything below is the actual patch v3----- 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-pc-linux-gnu. 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: * semantics.cc (maybe_convert_cond): Handle the implicit operator=() case for -Wparentheses. gcc/testsuite/ChangeLog: * g++.dg/warn/Wparentheses-31.C: New test. --- gcc/cp/semantics.cc | 21 +++++++- gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 55 +++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 0cb17a6a8ab..30ffb23a032 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -815,6 +815,25 @@ finish_goto_stmt (tree destination) return add_stmt (build_stmt (input_location, GOTO_EXPR, destination)); } +/* 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; + + tree fn = TREE_OPERAND (expr, 0); + if (TREE_CODE (fn) != CALL_EXPR || !CALL_EXPR_OPERATOR_SYNTAX (fn)) + return false; + + tree fndecl = cp_get_callee_fndecl_nofold (fn); + return fndecl != NULL_TREE + && DECL_OVERLOADED_OPERATOR_P (fndecl) + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR) + && DECL_ASSIGNMENT_OPERATOR_P (fndecl); +} + /* 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 +855,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_overload_ref_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..7a5789fb7a1 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C @@ -0,0 +1,55 @@ +/* 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); + operator bool (); +}; + +struct B +{ + bool x; + B& operator= (int); + operator bool (); +}; + +struct C +{ + C& operator= (int); + virtual C& operator= (double); + operator bool (); +}; + +void f1 (A a1, A a2) +{ + 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); +} + +void f2(B b1, B b2) +{ + 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)); +} + +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)); +}
On 2/12/22 01:59, Zhao Wei Liew wrote: > On Fri, 11 Feb 2022 at 20:47, Jason Merrill <jason@redhat.com> wrote: >>> >>> On the other hand, for empty classes, it seems that a COMPOUND_EXPR >>> is built in build_over_call under the is_really_empty_class guard (line 9791). >>> I don't understand the tree structure that I should identify though. >>> Could you give me any further explanations on that? >> >> True, that's not very recognizable. I suppose we could use a >> TREE_LANG_FLAG to mark that COMPOUND_EXPR, or we could leave empty >> classes alone; neither assignment nor comparison of empty classes should >> happen much in practice. > > I agree. I'll leave them alone. > >>> >>> Got it. May I know why it's better to use *_nofold here? >> >> To avoid trying to do constexpr evaluation of the function operand when >> it's non-constant; in the interesting cases it won't be needed. >> >> However, have you tested virtual operator=? >> > > Yup, everything seems to work as expected for structs using virtual operator=. > I've updated the test suite to reflect the tests. > > One thing to note: I've commented out 2 test statements that shouldn't > work. One of them > is caused by trivial assignments in empty classes being COMPOUND_EXPRs as we > discussed above. The other is an existing issue caused by trivial > assignments in non-empty > classes being MODIFY_EXPRs. > >>> On an unrelated note, I adjusted the if condition to use INDIRECT_REF_P (cond) >>> instead of TREE_OPERAND_LENGTH (cond) >= 1. >>> I hope that's better for semantics. >> >> Yes; REFERENCE_REF_P might be even better. >> > > Alright, I've changed it to REFERENCE_REF_P and it seems to work fine as well. > > > > -----Everything below is the actual patch v3----- > > > > 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-pc-linux-gnu. > > 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: > > * semantics.cc (maybe_convert_cond): Handle the implicit > operator=() case for -Wparentheses. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wparentheses-31.C: New test. > --- > gcc/cp/semantics.cc | 21 +++++++- > gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 55 +++++++++++++++++++++ > 2 files changed, 75 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index 0cb17a6a8ab..30ffb23a032 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -815,6 +815,25 @@ finish_goto_stmt (tree destination) > return add_stmt (build_stmt (input_location, GOTO_EXPR, destination)); > } > > +/* 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. > + tree fn = TREE_OPERAND (expr, 0); > + if (TREE_CODE (fn) != CALL_EXPR || !CALL_EXPR_OPERATOR_SYNTAX (fn)) > + return false; > + > + tree fndecl = cp_get_callee_fndecl_nofold (fn); > + return fndecl != NULL_TREE > + && DECL_OVERLOADED_OPERATOR_P (fndecl) > + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR) > + && DECL_ASSIGNMENT_OPERATOR_P (fndecl); This could be && DECL_ASSIGNMENT_OPERATOR_P (fndecl) && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); without the separate DECL_OVERLOADED_OPERATOR_P test. > +} > + > /* 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 +855,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_overload_ref_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..7a5789fb7a1 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C > @@ -0,0 +1,55 @@ > +/* 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); > + operator bool (); > +}; > + > +struct B > +{ > + bool x; > + B& operator= (int); > + operator bool (); > +}; > + > +struct C > +{ > + C& operator= (int); > + virtual C& operator= (double); > + operator bool (); > +}; > + > +void f1 (A a1, A a2) > +{ > + 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); > +} > + > +void f2(B b1, B b2) > +{ > + 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)); > +} > + > +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)); > +}
On 14/02/2022, Jason Merrill <jason@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. >> + return fndecl != NULL_TREE >> + && DECL_OVERLOADED_OPERATOR_P (fndecl) >> + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR) >> + && DECL_ASSIGNMENT_OPERATOR_P (fndecl); > > This could be > > && DECL_ASSIGNMENT_OPERATOR_P (fndecl) > && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); > > without the separate DECL_OVERLOADED_OPERATOR_P test. > I see. I've adjusted the code as you suggested and it works. Thanks! ------ Everything below is the patch v4 ------ 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-pc-linux-gnu. 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: * semantics.cc (maybe_convert_cond): Handle the implicit operator=() case for -Wparentheses. gcc/testsuite/ChangeLog: * g++.dg/warn/Wparentheses-31.C: New test. --- gcc/cp/semantics.cc | 29 +++++++++- gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 0cb17a6a8ab1c..9c3f613a1aa62 100644 --- 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) +{ + if (call == NULL_TREE) + return false; + + /* Unwrap the CALL_EXPR. */ + while (TREE_CODE (call) == COMPOUND_EXPR) + call = TREE_OPERAND (call, 1); + if (REFERENCE_REF_P (call)) + call = TREE_OPERAND (call, 0); + if (TREE_CODE (call) == TARGET_EXPR) + call = TARGET_EXPR_INITIAL (call); + + if (call == NULL_TREE + || TREE_CODE (call) != CALL_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 +863,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
On 2/14/22 21:30, Zhao Wei Liew wrote: > On 14/02/2022, Jason Merrill <jason@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. >>> + return fndecl != NULL_TREE >>> + && DECL_OVERLOADED_OPERATOR_P (fndecl) >>> + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR) >>> + && DECL_ASSIGNMENT_OPERATOR_P (fndecl); >> >> This could be >> >> && DECL_ASSIGNMENT_OPERATOR_P (fndecl) >> && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); >> >> without the separate DECL_OVERLOADED_OPERATOR_P test. >> > > I see. I've adjusted the code as you suggested and it works. Thanks! > > ------ Everything below is the patch v4 ------ > > 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-pc-linux-gnu. > > 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: > > * semantics.cc (maybe_convert_cond): Handle the implicit > operator=() case for -Wparentheses. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wparentheses-31.C: New test. > --- > gcc/cp/semantics.cc | 29 +++++++++- > gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++ > 2 files changed, 90 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index 0cb17a6a8ab1c..9c3f613a1aa62 100644 > --- 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 ( > +{ > + if (call == NULL_TREE) > + return false; > + > + /* Unwrap the CALL_EXPR. */ > + while (TREE_CODE (call) == COMPOUND_EXPR) > + call = TREE_OPERAND (call, 1); > + if (REFERENCE_REF_P (call)) > + call = TREE_OPERAND (call, 0); > + if (TREE_CODE (call) == TARGET_EXPR) > + call = TARGET_EXPR_INITIAL (call); > + > + if (call == NULL_TREE > + || TREE_CODE (call) != CALL_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 +863,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 >
On Tue, 15 Feb 2022 at 13:13, Jason Merrill <jason@redhat.com> wrote: > > On 2/14/22 21:30, Zhao Wei Liew wrote: > > On 14/02/2022, Jason Merrill <jason@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
On 2/15/22 05:06, Zhao Wei Liew wrote: > On Tue, 15 Feb 2022 at 13:13, Jason Merrill <jason@redhat.com> wrote: >> >> On 2/14/22 21:30, Zhao Wei Liew wrote: >>> On 14/02/2022, Jason Merrill <jason@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? I think instead of factoring out a new function, let's change the assert to an if and return NULL_TREE if it fails. >>> --- 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 >
On 2/15/22 07:48, Jason Merrill wrote: > On 2/15/22 05:06, Zhao Wei Liew wrote: >> On Tue, 15 Feb 2022 at 13:13, Jason Merrill <jason@redhat.com> wrote: >>> >>> On 2/14/22 21:30, Zhao Wei Liew wrote: >>>> On 14/02/2022, Jason Merrill <jason@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? > > I think instead of factoring out a new function, let's change the assert > to an if and return NULL_TREE if it fails. Incidentally, the subject should be "c++:" instead of "c:". 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. 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. Jason >>>> --- 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), >> 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 >> >
On Tue, 15 Feb 2022 at 21:05, Jason Merrill <jason@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 <zhaoweiliew@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)); +}
On Wed, 16 Feb 2022 at 00:30, Zhao Wei Liew <zhaoweiliew@gmail.com> wrote: > On Tue, 15 Feb 2022 at 21:05, Jason Merrill <jason@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 <zhaoweiliew@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 > >
On 2/15/22 11:30, Zhao Wei Liew wrote: > On Tue, 15 Feb 2022 at 21:05, Jason Merrill <jason@redhat.com > <mailto:jason@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. That came from this line in the testcase: > +/* PR c/25689 */ The PR should be c++/25689. Also, sometimes the bugzilla component isn't the same as the area of the compiler you're changing; the latter is what you want in the patch subject, so that the right people know to review it. > > 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 <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. FWIW it's often easier to send the patch as an attachment. > v5: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590393.html > <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 > <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 > <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 > <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 > <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 <zhaoweiliew@gmail.com <mailto:zhaoweiliew@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; Note that since this can return error_mark_node... > } > > 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)) ...you probably want to check for it here. > + 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)); You can avoid it by calling suppress_warning on that MODIFY_EXPR in build_over_call. > +} > + > +/* 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 >
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 466d6b56871f4..6a25d039585f2 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -836,7 +836,19 @@ maybe_convert_cond (tree cond) /* Do the conversion. */ cond = convert_from_reference (cond); - if (TREE_CODE (cond) == MODIFY_EXPR + /* Also check if this is a call to operator=(). + Example: if (my_struct = 5) {...} + */ + tree fndecl = NULL_TREE; + if (TREE_OPERAND_LENGTH(cond) >= 1) { + fndecl = cp_get_callee_fndecl(TREE_OPERAND(cond, 0)); + } + + if ((TREE_CODE (cond) == MODIFY_EXPR + || (fndecl != NULL_TREE + && DECL_OVERLOADED_OPERATOR_P(fndecl) + && DECL_OVERLOADED_OPERATOR_IS(fndecl, NOP_EXPR) + && DECL_ASSIGNMENT_OPERATOR_P(fndecl))) && 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..abd7476ccb461 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C @@ -0,0 +1,11 @@ +/* PR c/25689 */ +/* { dg-options "-Wparentheses" } */ + +struct A { + A& operator=(int); + operator bool(); +}; + +void f(A a) { + if (a = 0); /* { dg-warning "suggest parentheses" } */ +}