From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id F25C03858C20 for ; Fri, 11 Feb 2022 12:47:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F25C03858C20 Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-237-UFeGHlaIMTK4U-73XcTxZw-1; Fri, 11 Feb 2022 07:47:02 -0500 X-MC-Unique: UFeGHlaIMTK4U-73XcTxZw-1 Received: by mail-qk1-f198.google.com with SMTP id bj2-20020a05620a190200b005084968bb24so5447595qkb.23 for ; Fri, 11 Feb 2022 04:47:02 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=GYr6t0GFjZs45ZCMgyLv+5u3kAJ+gzM/f9Z5zc3NduE=; b=ueD2tRJRVwfIZwIBjtYrwfpGwG4QsmvZ7WpRiDTi7dg+M+GiHC8fv2Z4nTwOfHd0S9 9Zaezqzv9jrpsKV7SGlcwihtEhEEYzU9EhvZJ3T+M9fFDCVZMbMAsCTOk1SROgef5zh6 lu7dpKy/P4hNvPsM2a1uSxQLztDlYRJ8q+/Om4LZgB/t11I46pYwoc+NUAMdP6qljptF T/FWoTKS3DdVfz3EnxNfV18h2OU5zc1VHWuoIOklC1BRFl1bpPht4jp7kFxY2Af4F2KA 5UbVw0D+Godz2IYASvEewA2B1Y06zK8osOMXXrxvNCQ5j9UZaVuAhPZkIt6175YMtQ3G +0YQ== X-Gm-Message-State: AOAM530fwJjX4QUgJjNiD4lCbi02JX9Q3mgUAEuCXPCyzsfXIC60/9tC aHaFYeMPHadHduPZbLHsoxL5TvyJ6+5jvNTlYMWxsTy4gALiG5HdHtUHgP2RIL1nit0K99XlExo TUx1JImIxEcDXELj2iQ== X-Received: by 2002:a05:622a:190d:: with SMTP id w13mr936478qtc.74.1644583622111; Fri, 11 Feb 2022 04:47:02 -0800 (PST) X-Google-Smtp-Source: ABdhPJw+7GmrmexfKbkp6FiRAGwcxHcvAGYt5aHhfJ8dBDRhWjBW0r+Q9eQqDxqun/0E2RQQVAfgNQ== X-Received: by 2002:a05:622a:190d:: with SMTP id w13mr936458qtc.74.1644583621714; Fri, 11 Feb 2022 04:47:01 -0800 (PST) Received: from [192.168.1.149] (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id f16sm13201682qtk.8.2022.02.11.04.47.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 11 Feb 2022 04:47:00 -0800 (PST) Message-ID: <56773657-7772-6bbf-f1ac-4d5a94492c84@redhat.com> Date: Fri, 11 Feb 2022 07:47:00 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.1 Subject: Re: [PATCH] c: Add diagnostic when operator= is used as truth cond [PR25689] To: Zhao Wei Liew Cc: GCC Patches References: From: Jason Merrill In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, 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 12:47:11 -0000 On 2/10/22 23:01, Zhao Wei Liew wrote: > 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? 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 >