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 5A64D3858D28 for ; Sat, 6 Aug 2022 22:49:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5A64D3858D28 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-41-1yyJWWmhN3-MP7XHMe2Umg-1; Sat, 06 Aug 2022 18:49:16 -0400 X-MC-Unique: 1yyJWWmhN3-MP7XHMe2Umg-1 Received: by mail-qk1-f198.google.com with SMTP id bm34-20020a05620a19a200b006b5f1d95ceeso4937773qkb.5 for ; Sat, 06 Aug 2022 15:49:15 -0700 (PDT) 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:references:from:in-reply-to :content-transfer-encoding; bh=HGbkGvr/ErLzjm7JVt50b3m3JDDxngdBE0a9F6Hde14=; b=lerIm4t7T1G7730t4sbKWAlhq23KrurvpfbwFzTm4ZjeRXT2s6aLfn+ibnujnE45Nk txzOQL84W/Zw1J2DKbaPbjz0Jy6lhGL2tqcrjHQnec9Vn35zf+ocfkd/ppEyc7U1j/2B wGEk7FnaQ3tolWPLb9Ccy/Wnv4+teoInne+q8h0HbVKNzwwcAGZmeN61VNfmJqOPsufY afPBmR0gNIkJVP0gdUiEUI2DwklCCVM9eRFoKXNIdHCiEaNWJBg8Nh82L3gWjuikvPhe HiC3Zztg3E0bpEKg6pHp6KkqTEAXbqilztH5Oi+iFEewgZp7fPkELOZ28t/g26U/9uvV eZJA== X-Gm-Message-State: ACgBeo3XUwfPlb+h5dVr4E931HyJtlD+yT5Tiu17pZRZdJEXxICuRmNp Qi3DuC9qhEORKhmsZ+yS46xm/nXXrATGoqq5CDUo46OhXYouhY7mrRTF/wxG6QT0LfqRspgIAIn q+LoGh0+lequKMje1SQ== X-Received: by 2002:a05:620a:2848:b0:6af:6c3f:7141 with SMTP id h8-20020a05620a284800b006af6c3f7141mr9735463qkp.548.1659826155346; Sat, 06 Aug 2022 15:49:15 -0700 (PDT) X-Google-Smtp-Source: AA6agR45ZP7ScEZBcpmm6dATmo83IZa/AE2ex2jJJU4OviQE3p7qzN+1U8dXi8MZGtbUm+nzNKLrPA== X-Received: by 2002:a05:620a:2848:b0:6af:6c3f:7141 with SMTP id h8-20020a05620a284800b006af6c3f7141mr9735457qkp.548.1659826155001; Sat, 06 Aug 2022 15:49:15 -0700 (PDT) Received: from [192.168.44.44] ([72.2.237.35]) by smtp.gmail.com with ESMTPSA id i25-20020ac84899000000b0033ff6387ab8sm5010521qtq.57.2022.08.06.15.49.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 06 Aug 2022 15:49:14 -0700 (PDT) Message-ID: <50df8e96-3894-b9d2-dbce-a886c04e9815@redhat.com> Date: Sat, 6 Aug 2022 15:49:04 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [PATCH] c++: Extend -Wpessimizing-move for class prvalues [PR106276] To: Marek Polacek , GCC Patches References: <20220728001424.396849-1-polacek@redhat.com> From: Jason Merrill In-Reply-To: <20220728001424.396849-1-polacek@redhat.com> 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.5 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.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-List-Received-Date: Sat, 06 Aug 2022 22:49:21 -0000 On 7/27/22 17:14, Marek Polacek wrote: > We already have a warning that warns about pessimizing std::move > in a return statement, when it prevents the NRVO: > > T fn() > { > T t; > return std::move (t); // warning \o/ > } > > However, the warning doesn't warn when what we are returning is a class > prvalue, that is, when std::move prevents the RVO: > > T fn() > { > T t; > return std::move (T{}); // no warning :-( > } > > This came up recently in GCC: > . > > This patch fixes that. I would like to extend the warning further, so > that it warns in more contexts, e.g.: > > T t = std::move(T()); > > or > > void foo (T); > foo (std::move(T())); > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > PR c++/106276 > > gcc/cp/ChangeLog: > > * typeck.cc (can_do_rvo_p): New. > (maybe_warn_pessimizing_move): Warn when moving a temporary object > in a return statement prevents copy elision. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/Wpessimizing-move7.C: New test. > --- > gcc/cp/typeck.cc | 31 ++++++++- > .../g++.dg/cpp0x/Wpessimizing-move7.C | 63 +++++++++++++++++++ > 2 files changed, 91 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index 6e4f23af982..9500c4e2fe8 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -10287,12 +10287,29 @@ can_do_nrvo_p (tree retval, tree functype) > /* The cv-unqualified type of the returned value must be the > same as the cv-unqualified return type of the > function. */ > - && same_type_p ((TYPE_MAIN_VARIANT (TREE_TYPE (retval))), > - (TYPE_MAIN_VARIANT (functype))) > + && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (retval)), > + TYPE_MAIN_VARIANT (functype)) > /* And the returned value must be non-volatile. */ > && !TYPE_VOLATILE (TREE_TYPE (retval))); > } > > +/* Like can_do_nrvo_p, but we check if we're trying to move a class > + prvalue. */ > + > +static bool > +can_do_rvo_p (tree retval, tree functype) > +{ > + if (functype == error_mark_node) > + return false; > + if (retval) > + STRIP_ANY_LOCATION_WRAPPER (retval); > + return (retval != NULL_TREE > + && TREE_CODE (retval) == TARGET_EXPR Maybe use !glvalue_p instead of specifically checking for TARGET_EXPR? I don't feel strongly about this. > + && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (retval)), > + TYPE_MAIN_VARIANT (functype)) > + && !TYPE_VOLATILE (TREE_TYPE (retval))); > +} > + > /* If we should treat RETVAL, an expression being returned, as if it were > designated by an rvalue, returns it adjusted accordingly; otherwise, returns > NULL_TREE. See [class.copy.elision]. RETURN_P is true if this is a return > @@ -10401,12 +10418,20 @@ maybe_warn_pessimizing_move (tree retval, tree functype) > "prevents copy elision")) > inform (loc, "remove % call"); > } > + else if (can_do_rvo_p (arg, functype)) > + { > + auto_diagnostic_group d; > + if (warning_at (loc, OPT_Wpessimizing_move, > + "moving a temporary object in a return statement " > + "prevents copy elision")) It doesn't just prevent copy elision, it produces a dangling reference. This is a special case of the warning we talked about passing a temporary to a function that returns a reference argument unchanged, and should probably use a different warning flag. > + inform (loc, "remove % call"); > + } > /* Warn if the move is redundant. It is redundant when we would > do maybe-rvalue overload resolution even without std::move. */ > else if (warn_redundant_move > && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true))) > { > - /* Make sure that the overload resolution would actually succeed > + /* Make sure that overload resolution would actually succeed > if we removed the std::move call. */ > tree t = convert_for_initialization (NULL_TREE, functype, > moved, > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > new file mode 100644 > index 00000000000..cd4eaa09aae > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > @@ -0,0 +1,63 @@ > +// PR c++/106276 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wpessimizing-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); } > +} > + > +struct A { A(); A(const A&) = delete; A(A&&); }; > +struct B { B(A); }; > + > +static A foo (); > + > +A > +fn1 () > +{ > + return std::move (A{}); // { dg-warning "moving a temporary object in a return statement prevents copy elision" } > + return std::move (A()); // { dg-warning "moving a temporary object in a return statement prevents copy elision" } > + return std::move (foo ()); // { dg-warning "moving a temporary object in a return statement prevents copy elision" } > +} > + > +B fn2 () > +{ > + return std::move (A()); > + return std::move (A{}); > + return std::move (foo ()); > +} > + > +template > +T1 > +fn3 () > +{ > + return std::move (T2{}); // { dg-warning "moving a temporary object in a return statement prevents copy elision" } > +} > + > +void > +do_fn3 () > +{ > + fn3(); > + fn3(); > +} > + > +char take_buffer; > +struct label_text { > + label_text take() { return std::move(label_text(&take_buffer)); } // { dg-warning "moving a temporary object in a return statement prevents copy elision" } > + label_text(char *); > +}; > > base-commit: 219f86495791fd27b012678f13e10cada211daab