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 066A23858C2D for ; Tue, 16 Aug 2022 19:23:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 066A23858C2D Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-671-HfC4kyr3O8GuxlwaLxQp2Q-1; Tue, 16 Aug 2022 15:23:20 -0400 X-MC-Unique: HfC4kyr3O8GuxlwaLxQp2Q-1 Received: by mail-qv1-f69.google.com with SMTP id dc13-20020a056214174d00b0047b6f9a1a9aso4967768qvb.23 for ; Tue, 16 Aug 2022 12:23:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc; bh=tX6HVNNOOmPttjREKTbSNtKzvevqDao/TuZX7ikpFEI=; b=ixJ5/WkeX5QdMae0FTNy17dcYxK8NAglyyz7xJKqlCfAm+Ps5dG2D2SrxPA0AWNMRI qTgdi7SaOQXpCSQomSF1nrpvOd0IhaRdTycl1JjQ0nYUHxMykjKDuWVB2rKpqFs0tzky aZt/JyXPZzLrXQ4ZnZa8DBW0iF5fT9rY2u1ZBeScXKg6CtDB/yiMCwnBvRvV1vyue9Eq /DStQvGl3E0HhoEXfqLeEt8PDxkHCcZBv/wJTC8OTGHIXgXTiYfJRt2eheVtBgD/jEI+ loo6Zv1b7pbMpRJVrY7xfJ4xvrmljgH2IYmwg6KTjezK7GOKLKkcyIzcCKaajqufLIbP OweA== X-Gm-Message-State: ACgBeo1RmIt/d78vUQ8NittG3l67CfhCvm+mj3IDB6hBO60Cft981nT9 3B58eenxUOEFWMSS9dBDt+wK9gOS4wti+RXvrQf/WntwlW7IEG4UulFPTJkTl7U7O7y16/LM9yw x4D4Cqtr05H+XfQR7+A== X-Received: by 2002:a05:620a:1aa4:b0:6bb:54b7:bdaf with SMTP id bl36-20020a05620a1aa400b006bb54b7bdafmr5388764qkb.575.1660677800012; Tue, 16 Aug 2022 12:23:20 -0700 (PDT) X-Google-Smtp-Source: AA6agR74mqrwybC2Cj5qvkfDlLc+Q8Kbcuj/SktLMfrMNPCM6xQYHdNsDTqIFWGIIfme47a5WRsaSw== X-Received: by 2002:a05:620a:1aa4:b0:6bb:54b7:bdaf with SMTP id bl36-20020a05620a1aa400b006bb54b7bdafmr5388746qkb.575.1660677799611; Tue, 16 Aug 2022 12:23:19 -0700 (PDT) Received: from [192.168.1.101] (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 w7-20020a05620a424700b006b9593e2f68sm12450896qko.4.2022.08.16.12.23.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 16 Aug 2022 12:23:19 -0700 (PDT) Message-ID: <9a065efc-c478-8dbf-3251-afd50891e152@redhat.com> Date: Tue, 16 Aug 2022 15:23:18 -0400 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 to other contexts To: Marek Polacek , GCC Patches References: <20220802230447.721163-1-polacek@redhat.com> From: Jason Merrill In-Reply-To: <20220802230447.721163-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=-13.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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: Tue, 16 Aug 2022 19:23:25 -0000 On 8/2/22 16:04, Marek Polacek wrote: > In my recent patch which enhanced -Wpessimizing-move so that it warns > about class prvalues too I said that I'd like to extend it so that it > warns in more contexts where a std::move can prevent copy elision, such > as: > > T t = std::move(T()); > T t(std::move(T())); > T t{std::move(T())}; > T t = {std::move(T())}; > void foo (T); > foo (std::move(T())); > > This patch does that by adding two maybe_warn_pessimizing_move calls. > These must happen before we've converted the initializers otherwise the > std::move will be buried in a TARGET_EXPR. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > PR c++/106276 > > gcc/cp/ChangeLog: > > * call.cc (build_over_call): Call maybe_warn_pessimizing_move. > * cp-tree.h (maybe_warn_pessimizing_move): Declare. > * decl.cc (build_aggr_init_full_exprs): Call > maybe_warn_pessimizing_move. > * typeck.cc (maybe_warn_pessimizing_move): Handle TREE_LIST and > CONSTRUCTOR. Add a bool parameter and use it. Adjust a diagnostic > message. > (check_return_expr): Adjust the call to maybe_warn_pessimizing_move. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/Wpessimizing-move7.C: Add dg-warning. > * g++.dg/cpp0x/Wpessimizing-move8.C: New test. > --- > gcc/cp/call.cc | 5 +- > gcc/cp/cp-tree.h | 1 + > gcc/cp/decl.cc | 3 +- > gcc/cp/typeck.cc | 58 ++++++++++++----- > .../g++.dg/cpp0x/Wpessimizing-move7.C | 16 ++--- > .../g++.dg/cpp0x/Wpessimizing-move8.C | 65 +++++++++++++++++++ > 6 files changed, 120 insertions(+), 28 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc > index 01a7be10077..370137ebd6d 100644 > --- a/gcc/cp/call.cc > +++ b/gcc/cp/call.cc > @@ -9627,10 +9627,13 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) > if (!conversion_warning) > arg_complain &= ~tf_warning; > > + if (arg_complain & tf_warning) > + maybe_warn_pessimizing_move (arg, type, /*return_p*/false); > + > val = convert_like_with_context (conv, arg, fn, i - is_method, > arg_complain); > val = convert_for_arg_passing (type, val, arg_complain); > - > + > if (val == error_mark_node) > return error_mark_node; > else > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index 3278b4114bd..5a8af22b509 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -8101,6 +8101,7 @@ extern tree finish_right_unary_fold_expr (tree, int); > extern tree finish_binary_fold_expr (tree, tree, int); > extern tree treat_lvalue_as_rvalue_p (tree, bool); > extern bool decl_in_std_namespace_p (tree); > +extern void maybe_warn_pessimizing_move (tree, tree, bool); > > /* in typeck2.cc */ > extern void require_complete_eh_spec_types (tree, tree); > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index 70ad681467e..dc6853a7de1 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -7220,9 +7220,10 @@ check_array_initializer (tree decl, tree type, tree init) > > static tree > build_aggr_init_full_exprs (tree decl, tree init, int flags) > - > { > gcc_assert (stmts_are_full_exprs_p ()); > + if (init) > + maybe_warn_pessimizing_move (init, TREE_TYPE (decl), /*return_p*/false); This is a surprising place to add this. Why here rather than in build_aggr_init or check_initializer? > return build_aggr_init (decl, init, flags, tf_warning_or_error); > } > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index 9500c4e2fe8..2650beb780e 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -10368,17 +10368,17 @@ treat_lvalue_as_rvalue_p (tree expr, bool return_p) > } > } > > -/* Warn about wrong usage of std::move in a return statement. RETVAL > - is the expression we are returning; FUNCTYPE is the type the function > - is declared to return. */ > +/* Warn about dubious usage of std::move (in a return statement, if RETURN_P > + is true). EXPR is the std::move expression; TYPE is the type of the object > + being initialized. */ > > -static void > -maybe_warn_pessimizing_move (tree retval, tree functype) > +void > +maybe_warn_pessimizing_move (tree expr, tree type, bool return_p) > { > if (!(warn_pessimizing_move || warn_redundant_move)) > return; > > - location_t loc = cp_expr_loc_or_input_loc (retval); > + const location_t loc = cp_expr_loc_or_input_loc (expr); > > /* C++98 doesn't know move. */ > if (cxx_dialect < cxx11) > @@ -10390,14 +10390,32 @@ maybe_warn_pessimizing_move (tree retval, tree functype) > return; > > /* This is only interesting for class types. */ > - if (!CLASS_TYPE_P (functype)) > + if (!CLASS_TYPE_P (type)) > return; > > + /* A a = std::move (A()); */ > + if (TREE_CODE (expr) == TREE_LIST) > + { > + if (list_length (expr) == 1) > + expr = TREE_VALUE (expr); > + else > + return; > + } > + /* A a = {std::move (A())}; > + A a{std::move (A())}; */ > + else if (TREE_CODE (expr) == CONSTRUCTOR) > + { > + if (CONSTRUCTOR_NELTS (expr) == 1) > + expr = CONSTRUCTOR_ELT (expr, 0)->value; > + else > + return; > + } > + > /* We're looking for *std::move ((T &) &arg). */ > - if (REFERENCE_REF_P (retval) > - && TREE_CODE (TREE_OPERAND (retval, 0)) == CALL_EXPR) > + if (REFERENCE_REF_P (expr) > + && TREE_CODE (TREE_OPERAND (expr, 0)) == CALL_EXPR) > { > - tree fn = TREE_OPERAND (retval, 0); > + tree fn = TREE_OPERAND (expr, 0); > if (is_std_move_p (fn)) > { > tree arg = CALL_EXPR_ARG (fn, 0); > @@ -10409,20 +10427,24 @@ maybe_warn_pessimizing_move (tree retval, tree functype) > return; > arg = TREE_OPERAND (arg, 0); > arg = convert_from_reference (arg); > - /* Warn if we could do copy elision were it not for the move. */ > - if (can_do_nrvo_p (arg, functype)) > + if (can_do_rvo_p (arg, type)) > { > auto_diagnostic_group d; > if (warning_at (loc, OPT_Wpessimizing_move, > - "moving a local object in a return statement " > - "prevents copy elision")) > + "moving a temporary object prevents copy " > + "elision")) > inform (loc, "remove % call"); > } > - else if (can_do_rvo_p (arg, functype)) > + /* The rest of the warnings is only relevant for when we are > + returning from a function. */ > + else if (!return_p) > + return; > + /* Warn if we could do copy elision were it not for the move. */ > + else if (can_do_nrvo_p (arg, type)) > { > auto_diagnostic_group d; > if (warning_at (loc, OPT_Wpessimizing_move, > - "moving a temporary object in a return statement " > + "moving a local object in a return statement " > "prevents copy elision")) > inform (loc, "remove % call"); > } > @@ -10433,7 +10455,7 @@ maybe_warn_pessimizing_move (tree retval, tree functype) > { > /* Make sure that overload resolution would actually succeed > if we removed the std::move call. */ > - tree t = convert_for_initialization (NULL_TREE, functype, > + tree t = convert_for_initialization (NULL_TREE, type, > moved, > (LOOKUP_NORMAL > | LOOKUP_ONLYCONVERTING > @@ -10718,7 +10740,7 @@ check_return_expr (tree retval, bool *no_warning) > return NULL_TREE; > > if (!named_return_value_okay_p) > - maybe_warn_pessimizing_move (retval, functype); > + maybe_warn_pessimizing_move (retval, functype, /*return_p*/true); > > /* Do any required conversions. */ > if (bare_retval == result || DECL_CONSTRUCTOR_P (current_function_decl)) > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > index cd4eaa09aae..a17c7a87b7d 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > @@ -30,23 +30,23 @@ 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" } > + return std::move (A{}); // { dg-warning "moving a temporary object prevents copy elision" } > + return std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" } > + return std::move (foo ()); // { dg-warning "moving a temporary object prevents copy elision" } > } > > B fn2 () > { > - return std::move (A()); > - return std::move (A{}); > - return std::move (foo ()); > + return std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" } > + return std::move (A{}); // { dg-warning "moving a temporary object prevents copy elision" } > + return std::move (foo ()); // { dg-warning "moving a temporary object prevents copy elision" } > } > > template > T1 > fn3 () > { > - return std::move (T2{}); // { dg-warning "moving a temporary object in a return statement prevents copy elision" } > + return std::move (T2{}); // { dg-warning "moving a temporary object prevents copy elision" } > } > > void > @@ -58,6 +58,6 @@ do_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 take() { return std::move(label_text(&take_buffer)); } // { dg-warning "moving a temporary object prevents copy elision" } > label_text(char *); > }; > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C > new file mode 100644 > index 00000000000..51406c8f97f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C > @@ -0,0 +1,65 @@ > +// 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); }; > +struct X { }; > + > +void foo (A); > +void bar (X); > + > +void > +fn1 () > +{ > + A a1 = std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" } > + A a2 = std::move (A{}); // { dg-warning "moving a temporary object prevents copy elision" } > + A a3(std::move (A())); // { dg-warning "moving a temporary object prevents copy elision" } > + A a4(std::move (A{})); // { dg-warning "moving a temporary object prevents copy elision" } > + A a5{std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" } > + A a6{std::move (A{})}; // { dg-warning "moving a temporary object prevents copy elision" } > + A a7 = {std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" } > + A a8 = {std::move (A{})}; // { dg-warning "moving a temporary object prevents copy elision" } > + > + B b1 = std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" } > + B b2(std::move (A())); // { dg-warning "moving a temporary object prevents copy elision" } > + B b3{std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" } > + B b4 = {std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" } > + > + X x1 = std::move (X()); // { dg-warning "moving a temporary object prevents copy elision" } > + X x2 = std::move (X{}); // { dg-warning "moving a temporary object prevents copy elision" } > + X x3(std::move (X())); // { dg-warning "moving a temporary object prevents copy elision" } > + X x4(std::move (X{})); // { dg-warning "moving a temporary object prevents copy elision" } > + X x5{std::move (X())}; // { dg-warning "moving a temporary object prevents copy elision" } > + X x6{std::move (X{})}; // { dg-warning "moving a temporary object prevents copy elision" } > + X x7 = {std::move (X())}; // { dg-warning "moving a temporary object prevents copy elision" } > + X x8 = {std::move (X{})}; // { dg-warning "moving a temporary object prevents copy elision" } > + > + foo (std::move (A())); // { dg-warning "moving a temporary object prevents copy elision" } > + foo (std::move (A{})); // { dg-warning "moving a temporary object prevents copy elision" } > + bar (std::move (X())); // { dg-warning "moving a temporary object prevents copy elision" } > + bar (std::move (X{})); // { dg-warning "moving a temporary object prevents copy elision" } > + > + foo (std::move (a1)); > + bar (std::move (x1)); > +} > > base-commit: 502605a277d36cee1b0570982a16d97a43eace67 > prerequisite-patch-id: f4862bc588ce5fed1d21016fecc4b61feb71eba5