From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd2e.google.com (mail-io1-xd2e.google.com [IPv6:2607:f8b0:4864:20::d2e]) by sourceware.org (Postfix) with ESMTPS id 5A34F3858D1E for ; Fri, 11 Feb 2022 04:01:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5A34F3858D1E Received: by mail-io1-xd2e.google.com with SMTP id m185so9942292iof.10 for ; Thu, 10 Feb 2022 20:01:35 -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:in-reply-to:references:from:date :message-id:subject:to:cc; bh=ps+Y8aINtERltj4JwjFryWwVs4+4rXFxxZfLALJAHAs=; b=AoUY3Dbkaeu0JUap14l38LLfEL4ofCtgGq6f4tcL/fJiazL90GUBOQQhQaEF0mkEqP ve0btzCgA0Fxtr2wqEggy9R9jvYIBh0WmqTc1Y2WEBqdxrFE+3ALAuKC7AfcJDEhOz4C u6XOaDdeM7d4npQyJbOy/81zDGC+JDtBNf84BxyoH9cUidBj6juA7gluHlWqVn5Tcl6x XM2pk5o8FBBXjczqZrGyh30kz7FCkVCJQs52Y3TYvMf4GxSRkHwd2dWx6TJtiExcnBO/ 88tbfXQcEiKIn2X/EElj1dC4X+sIH4Uj11zB9mmUBTwcz+RvvsSrYmg/sUgNCW29Vja3 Bv7A== X-Gm-Message-State: AOAM532gy5s7WrivGTBbySE4MDKEaWpYBhoV3r8MK3B4Ww/ZYDC5aNyN TLfwDK6nciYBrCrnD6hYNtzZHtxd5wegt8kDLyUrDWtLPYY= X-Google-Smtp-Source: ABdhPJyr9ApQu2ka/tT/ZL3TMiM0tu+r/tieFKNCKOMRK/TMkaG8JeKa7/VkYpk8jKC4wlA+CSb+2Rr3MYSYRkFmqGk= X-Received: by 2002:a6b:5a05:: with SMTP id o5mr5464364iob.19.1644552094532; Thu, 10 Feb 2022 20:01:34 -0800 (PST) MIME-Version: 1.0 Received: by 2002:a05:6e02:1584:0:0:0:0 with HTTP; Thu, 10 Feb 2022 20:01:34 -0800 (PST) In-Reply-To: References: From: Zhao Wei Liew Date: Fri, 11 Feb 2022 12:01:34 +0800 Message-ID: Subject: Re: [PATCH] c: Add diagnostic when operator= is used as truth cond [PR25689] To: Jason Merrill Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Feb 2022 04:01:37 -0000 On Fri, 11 Feb 2022 at 00:14, Jason Merrill 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