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 DD7123858418 for ; Tue, 16 Aug 2022 12:27:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DD7123858418 Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-617-hwOL1AyoOrK0tzUby6AtkQ-1; Tue, 16 Aug 2022 08:27:05 -0400 X-MC-Unique: hwOL1AyoOrK0tzUby6AtkQ-1 Received: by mail-qv1-f70.google.com with SMTP id oo27-20020a056214451b00b00477249248e2so4205744qvb.4 for ; Tue, 16 Aug 2022 05:27:05 -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:to:from:date:x-gm-message-state:from:to:cc; bh=IKwLAwIb7J7Qxpd2tqICa3qBFDaEPFkIbHR8o9A17VQ=; b=j5ZnROTAO5fRy08iK2b5yzGgw/WdBIezmDe3EuUcJic//bybcI67CxdgU+/r8k59eP 8PAV0FpPQOWp03hbg6NYagdoZ7noyxbAl7+yBBITsGk8WhJ9A/cNac+PPapVXwxHs+RP 1eUhG7+W6Ul9gWxSjs111Jh3fgXalCEmGMbwZBLb8SyM3tXsUps5zskEkz9w5TixVUD1 PzdSFr5GsQ2n228IL9lChpN4GvIdhkgylnILiuyNNDgUrdSRFjsxkWep8ue8ff4fgCLy W/3aNlQuw9Ixyr+kgcdlvv2kiysF0f0xwiUEwyzILL4rixhdWzwGsUn6DI9iFe+4iNS+ ObQw== X-Gm-Message-State: ACgBeo2NqGmkgaNc8SxDefomxM1ih7zbEV+bsr58RKS0L1iqzYbCi2t9 UBfJmgugLNFpcxCEjMg14R7VijB7bQSniam0t2vjuwer6BJosxyMmFw/vIwrrdK8up7Y6Cz4znt dgxLDhpdV5AB0/vRL+oshQK3EGucOEoQdloAIK3ldb8/pEXohKdWzdJXnM6m+iP/Amz4/ X-Received: by 2002:ad4:596e:0:b0:47a:2ae5:b65e with SMTP id eq14-20020ad4596e000000b0047a2ae5b65emr17169624qvb.109.1660652824787; Tue, 16 Aug 2022 05:27:04 -0700 (PDT) X-Google-Smtp-Source: AA6agR5z7E7N0s5KByda9TCu1lBQtDvJVqGRK+zFZg4yx9XcVqnDy0g0MJoE4rFeE6ajAwzpK0lQ8g== X-Received: by 2002:ad4:596e:0:b0:47a:2ae5:b65e with SMTP id eq14-20020ad4596e000000b0047a2ae5b65emr17169573qvb.109.1660652823962; Tue, 16 Aug 2022 05:27:03 -0700 (PDT) Received: from redhat.com ([2601:184:4780:4310::e531]) by smtp.gmail.com with ESMTPSA id w20-20020a05620a0e9400b006b928ba8989sm5916837qkm.23.2022.08.16.05.27.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Aug 2022 05:27:03 -0700 (PDT) Date: Tue, 16 Aug 2022 08:27:01 -0400 From: Marek Polacek To: GCC Patches , Jason Merrill Subject: Re: [PATCH] c++: Extend -Wpessimizing-move to other contexts Message-ID: References: <20220802230447.721163-1-polacek@redhat.com> MIME-Version: 1.0 In-Reply-To: <20220802230447.721163-1-polacek@redhat.com> User-Agent: Mutt/2.2.6 (2022-06-05) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-13.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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 12:27:09 -0000 Ping. (The other std::move patches depend on this one.) (can_do_rvo_p is renamed to can_elide_copy_prvalue_p in the PR90428 patch.) On Tue, Aug 02, 2022 at 07:04:47PM -0400, Marek Polacek via Gcc-patches 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); > 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 > -- > 2.37.1 > Marek