From patchwork Fri Aug 19 22:34:26 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 633 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:6a10:38f:b0:2d5:3c95:9e21 with SMTP id 15csp1218144pxh; Fri, 19 Aug 2022 15:35:18 -0700 (PDT) X-Google-Smtp-Source: AA6agR63QjLbRzzfmwGuBka0yi7KQ71hAW2QxhlCgZm6YWpRNVnS3iWmYuEAABqG5vf7879sdRFs X-Received: by 2002:a17:906:4790:b0:738:4c1c:ec60 with SMTP id cw16-20020a170906479000b007384c1cec60mr6120316ejc.523.1660948518019; Fri, 19 Aug 2022 15:35:18 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1660948518; cv=none; d=google.com; s=arc-20160816; b=MWl8uCqPwvq/S5hcbLX232TZFsQPwnSX1Sl64zoXh16qbu4elCHfxwpxCmtz8l9/QP KL+u9xOG8Hmmjd3EKpgBv560tWCYJB4qzAKdoSmahp0BIWDaDHokfg30x0OVUHKF6MnI wnwbgnk9DwcHge+M6uNFod7QJXxy5OpFyvKA9ne0uY3P5IBiAy+X0VQCVzUEQVlnsSLs MoG/Sh0sv60F/ajEdShCaviUSXPizrQ7cOHGo5LboxqXubgU/lrrldI5dh0JI9RI84aw A7fviu3r8VcKr6nDHFXC/LV5uStZJqp16KovSALXd3Bj4SS9TZhxrEujbNrHoLU1RubA yQyw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:cc:reply-to:from:list-subscribe:list-help :list-post:list-archive:list-unsubscribe:list-id:precedence :content-disposition:user-agent:in-reply-to:mime-version:references :message-id:subject:to:date:dmarc-filter:delivered-to:dkim-signature :dkim-filter; bh=ZR2I2vqRwR1c+n0O4m6/Rlm6Afx7ro0o6yomoxRNNwQ=; b=hMPtV5If0CKrYGssI4MvfPJuHSSDF45OY1hdHc9bK67euE46yw4Kf/3335VCKXd9dI 0sul57SGOyUm1NkIfVOWOrLLmBCgiOv0fAmWS5XNSv27n2uZ+3aayM3IGc+5DhHMCzU7 qM4M0ezRfDsnrEwYQPMUvFoOtZN4uhUX4JZuBs85KnwfHby7MZsCa3a+wS/yUrz/xq/d OKR7nDRn40+TYD5sS1n9hhaduDh4icSMtdLprSfUpASCslkiDhZS+0Ad9DqV2LrBRG3j B/Yych9FYGmedfhDCrAoooX7zX9k+W20fIVZtLY7G9hnNIU9MOzDzsg4sw24WO7Qz1Zj Neug== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=qgYqqGd9; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from sourceware.org (server2.sourceware.org. [2620:52:3:1:0:246e:9693:128c]) by mx.google.com with ESMTPS id dk10-20020a0564021d8a00b0043be0095146si3532466edb.601.2022.08.19.15.35.17 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 Aug 2022 15:35:18 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) client-ip=2620:52:3:1:0:246e:9693:128c; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=qgYqqGd9; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 31B413858C55 for ; Fri, 19 Aug 2022 22:35:16 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 31B413858C55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1660948516; bh=ZR2I2vqRwR1c+n0O4m6/Rlm6Afx7ro0o6yomoxRNNwQ=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=qgYqqGd9kQ1UY5SggjM3uvDKjxe1I89tCGxCXYeq9sHOPxeI4nuootokrQshnISbs LTCMHHknhyAvDgdAiSH6ENguNbvtZC1QWwWM1cu7X6L/hERlrDtaoJUJqB4XAY7nE/ TUBeNo3eq3OKftTbooabomLrq2EsaxIsghkktdOU= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 706373858D28 for ; Fri, 19 Aug 2022 22:34:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 706373858D28 Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-634-56J4aOm-Mz-RumsKFmF-Bw-1; Fri, 19 Aug 2022 18:34:29 -0400 X-MC-Unique: 56J4aOm-Mz-RumsKFmF-Bw-1 Received: by mail-qv1-f72.google.com with SMTP id ea4-20020ad458a4000000b0049682af0ca8so3507152qvb.21 for ; Fri, 19 Aug 2022 15:34:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc; bh=ZR2I2vqRwR1c+n0O4m6/Rlm6Afx7ro0o6yomoxRNNwQ=; b=cGF9DupPeQXD8wmij4fGAth/V3IwUJx6URUoS3Hm30SiZ/YzNS1o7lNwIlhB+VMkXP v5SgQdhUu6AX+PanPoGSSSuMeFQYthsPCmnxiZTpRitWGm0oGtR67bn5HPz1N5erhW2C Th89Z6LzZccsxSILUqM59ItDc3j2p3jQQWP+RGwsWB0qd0zlQbMi7Te4QMt10/2VB57p yPZ3aeZ33hukzQEva14hGN6GtSiZAcI1awiNDRbMZKTlqZ4PhEZqkRTfF6Mu/UJd4DA4 Fr4HXdBdTpKytqUCdzLcsWbzAb4+zR1bmJcaEdG6kJxtmzsUQIGUOUvSbC0aZOY8Qdxr pkZQ== X-Gm-Message-State: ACgBeo0LS9Uu0rsPxfSzIoAAhbdBMrsO8+kbiD1BppCAWl8HR2qyU0SJ +mYbcz5xUO4w5NGCMzdYu15Usc3jeWA+sY8G35RCpFAJ/qrbFzePb3BqBXCYTipW+pF7DIiGp0+ 8jqSx2/RTPjWxXJMIpg== X-Received: by 2002:a05:6214:4111:b0:476:d3b5:e6e6 with SMTP id kc17-20020a056214411100b00476d3b5e6e6mr8542013qvb.119.1660948469060; Fri, 19 Aug 2022 15:34:29 -0700 (PDT) X-Received: by 2002:a05:6214:4111:b0:476:d3b5:e6e6 with SMTP id kc17-20020a056214411100b00476d3b5e6e6mr8541991qvb.119.1660948468653; Fri, 19 Aug 2022 15:34:28 -0700 (PDT) Received: from redhat.com ([2601:184:4780:4310::e531]) by smtp.gmail.com with ESMTPSA id bj25-20020a05620a191900b006b93ef659c3sm4688142qkb.39.2022.08.19.15.34.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 Aug 2022 15:34:27 -0700 (PDT) Date: Fri, 19 Aug 2022 18:34:26 -0400 To: Jason Merrill Subject: [PATCH v3] c++: Implement -Wself-move warning [PR81159] Message-ID: References: <20220809163719.381319-1-polacek@redhat.com> <1ca5b35f-0fa6-be28-290f-701c5ac43807@redhat.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/2.2.6 (2022-06-05) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-23.1 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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-Patchwork-Original-From: Marek Polacek via Gcc-patches From: Marek Polacek Reply-To: Marek Polacek Cc: GCC Patches Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1740702315618280917?= X-GMAIL-MSGID: =?utf-8?q?1741630753307140889?= On Thu, Aug 18, 2022 at 08:33:47PM -0400, Jason Merrill wrote: > On 8/18/22 13:19, Marek Polacek wrote: > > On Mon, Aug 15, 2022 at 03:54:05PM -0400, Jason Merrill wrote: > > > On 8/9/22 09:37, Marek Polacek wrote: > > > > + /* We're looking for *std::move ((T &) &arg), or > > > > + *std::move ((T &) (T *) r) if the argument it a reference. */ > > > > + if (!REFERENCE_REF_P (rhs) > > > > + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) > > > > + return; > > > > + tree fn = TREE_OPERAND (rhs, 0); > > > > + if (!is_std_move_p (fn)) > > > > + return; > > > > + tree arg = CALL_EXPR_ARG (fn, 0); > > > > + if (TREE_CODE (arg) != NOP_EXPR) > > > > + return; > > > > + /* Strip the (T &). */ > > > > + arg = TREE_OPERAND (arg, 0); > > > > + /* Strip the (T *) or &. */ > > > > + arg = TREE_OPERAND (arg, 0); > > > > > > Are you sure these are the only two expressions that can make it here? What > > > if the argument to move is *Tptr? > > > > Not 100% sure but I couldn't find any other form. For *Tptr we get > > *std::move ((int * &) &Tptr) > > That likes like what you'd get when the argument is Tptr, not when it's > *Tptr. And indeed that's what I see in the testcase: > > > + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } > > is missing the * Duh, sorry. The previous patch didn't handle the *Tptr case. Further poking revealed that we need special care to handle (*(Tptr)) and **Tptr etc. So in this patch I'm stripping all *s and V_C_Es. Sigh. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- About 5 years ago we got a request to implement -Wself-move, which warns about useless moves like this: int x; x = std::move (x); This patch implements that warning. PR c++/81159 gcc/c-family/ChangeLog: * c.opt (Wself-move): New option. gcc/cp/ChangeLog: * typeck.cc (maybe_warn_self_move): New. (cp_build_modify_expr): Call maybe_warn_self_move. gcc/ChangeLog: * doc/invoke.texi: Document -Wself-move. gcc/testsuite/ChangeLog: * g++.dg/warn/Wself-move1.C: New test. --- gcc/c-family/c.opt | 4 + gcc/cp/typeck.cc | 51 +++++++++- gcc/doc/invoke.texi | 23 ++++- gcc/testsuite/g++.dg/warn/Wself-move1.C | 122 ++++++++++++++++++++++++ 4 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C base-commit: 713ec97e593bd4d9915a13bc4047f064fec0e24a diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index dfdebd596ef..f776efd39d8 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1229,6 +1229,10 @@ Wselector ObjC ObjC++ Var(warn_selector) Warning Warn if a selector has multiple methods. +Wself-move +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) +Warn when a value is moved to itself with std::move. + Wsequence-point C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn about possible violations of sequence point rules. diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index 992ebfd99fb..b2bd13db1b6 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -8897,7 +8897,54 @@ cp_build_c_cast (location_t loc, tree type, tree expr, return error_mark_node; } - + +/* Warn when a value is moved to itself with std::move. LHS is the target, + RHS may be the std::move call, and LOC is the location of the whole + assignment. */ + +static void +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) +{ + if (!warn_self_move) + return; + + /* C++98 doesn't know move. */ + if (cxx_dialect < cxx11) + return; + + if (processing_template_decl) + return; + + if (!REFERENCE_REF_P (rhs) + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) + return; + tree fn = TREE_OPERAND (rhs, 0); + if (!is_std_move_p (fn)) + return; + + /* Just a little helper to strip * and various NOPs. */ + auto extract_op = [] (tree &op) { + STRIP_NOPS (op); + while (INDIRECT_REF_P (op)) + op = TREE_OPERAND (op, 0); + op = maybe_undo_parenthesized_ref (op); + STRIP_ANY_LOCATION_WRAPPER (op); + }; + + tree arg = CALL_EXPR_ARG (fn, 0); + extract_op (arg); + if (TREE_CODE (arg) == ADDR_EXPR) + arg = TREE_OPERAND (arg, 0); + extract_op (lhs); + + if (cp_tree_equal (lhs, arg)) + { + auto_diagnostic_group d; + if (warning_at (loc, OPT_Wself_move, "moving a variable to itself")) + inform (loc, "remove % call"); + } +} + /* For use from the C common bits. */ tree build_modify_expr (location_t location, @@ -9101,6 +9148,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, if (modifycode == NOP_EXPR) { + maybe_warn_self_move (loc, lhs, rhs); + if (c_dialect_objc ()) { result = objc_maybe_build_modify_expr (lhs, rhs); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index f65d351a5fc..5dea3fee124 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -264,7 +264,7 @@ in the following sections. -Wreorder -Wregister @gol -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol -Wno-non-template-friend -Wold-style-cast @gol --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol -Wsized-deallocation -Wsuggest-final-methods @gol -Wsuggest-final-types -Wsuggest-override @gol -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol @@ -5843,6 +5843,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wreorder @gol -Wrestrict @gol -Wreturn-type @gol +-Wself-move @r{(only for C++)} @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol -Wsizeof-array-div @gol @@ -6828,6 +6829,26 @@ of a declaration: This warning is enabled by @option{-Wall}. +@item -Wno-self-move @r{(C++ and Objective-C++ only)} +@opindex Wself-move +@opindex Wno-self-move +This warning warns when a value is moved to itself with @code{std::move}. +Such a @code{std::move} typically has no effect. + +@smallexample +struct T @{ +@dots{} +@}; +void fn() +@{ + T t; + @dots{} + t = std::move (t); +@} +@end smallexample + +This warning is enabled by @option{-Wall}. + @item -Wsequence-point @opindex Wsequence-point @opindex Wno-sequence-point diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C new file mode 100644 index 00000000000..601d8c1521a --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C @@ -0,0 +1,122 @@ +// PR c++/81159 +// { dg-do compile { target c++11 } } +// { dg-options "-Wself-move" } + +// Define std::move. +namespace std { + template + struct remove_reference + { typedef _Tp type; }; + + template + struct remove_reference<_Tp&> + { typedef _Tp type; }; + + template + struct remove_reference<_Tp&&> + { typedef _Tp type; }; + + template + constexpr typename std::remove_reference<_Tp>::type&& + move(_Tp&& __t) noexcept + { return static_cast::type&&>(__t); } +} + +int g; + +struct S { + int x; + S(S&& o) { + x = std::move (x); // { dg-warning "moving a variable to itself" } + x = std::move (o.x); + o.x = std::move (x); + o.x = std::move (o.x); // { dg-warning "moving a variable to itself" } + } + void foo (int x) { + x = std::move (x); // { dg-warning "moving a variable to itself" } + } +}; + +struct X { + int x; + X(int x) : x(std::move (x)) { } +}; + +struct A {}; +struct B { A a; }; +struct C { C(); ~C(); }; +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; + +void +test () +{ + int i = 42; + i = std::move (i); // { dg-warning "moving a variable to itself" } + (i) = std::move (i); // { dg-warning "moving a variable to itself" } + + g = std::move (g); // { dg-warning "moving a variable to itself" } + (g) = std::move (g); // { dg-warning "moving a variable to itself" } + + A a; + a = std::move (a); // { dg-warning "moving a variable to itself" } + + B b; + b = std::move (b); // { dg-warning "moving a variable to itself" } + b.a = std::move (b.a); // { dg-warning "moving a variable to itself" } + + C c; + c = std::move (c); // { dg-warning "moving a variable to itself" } + D d; + d = std::move (d); // { dg-warning "moving a variable to itself" } +} + +template +void ttest () +{ + T t; + t = std::move (t); // { dg-warning "moving a variable to itself" } +} + +template void ttest(); + +void +testref (int &r, int &&rr) +{ + r = std::move (r); // { dg-warning "moving a variable to itself" } + rr = std::move (rr); // { dg-warning "moving a variable to itself" } +} + +// Test various other arguments to std::move. +template +void +testargs (T *Tptr, T **Tpptr, T& Tref, T&& Trref, const T *Tcptr) +{ + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } + *Tptr = std::move (*Tptr); // { dg-warning "moving a variable to itself" } + *Tptr = std::move (*(Tptr)); // { dg-warning "moving a variable to itself" } + *(Tptr) = std::move (*Tptr); // { dg-warning "moving a variable to itself" } + *(Tptr + 1) = std::move (*Tptr + 1); + *(Tptr + 1) = std::move (*Tptr + 2); + (*(Tptr)) = std::move (*Tptr); // { dg-warning "moving a variable to itself" } + *Tpptr = std::move (*Tpptr); // { dg-warning "moving a variable to itself" } + **Tpptr = std::move (**Tpptr); // { dg-warning "moving a variable to itself" } + Tref = std::move (Tref); // { dg-warning "moving a variable to itself" } + Trref = std::move (Trref); // { dg-warning "moving a variable to itself" } + Tcptr = std::move (Tcptr); // { dg-warning "moving a variable to itself" } + (Tptr) = std::move (Tptr); // { dg-warning "moving a variable to itself" } + (*Tptr) = std::move (*Tptr); // { dg-warning "moving a variable to itself" } + (*Tpptr) = std::move (*Tpptr); // { dg-warning "moving a variable to itself" } + (**Tpptr) = std::move (**Tpptr); // { dg-warning "moving a variable to itself" } + (*(*(Tpptr))) = std::move (**Tpptr); // { dg-warning "moving a variable to itself" } + (Tref) = std::move (Tref); // { dg-warning "moving a variable to itself" } + (Trref) = std::move (Trref); // { dg-warning "moving a variable to itself" } + (Tcptr) = std::move (Tcptr); // { dg-warning "moving a variable to itself" } +} + +void +call_testargs () +{ + int i = 42; + int *p = &i; + testargs(&i, &p, i, 42, &i); +}