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.133.124]) by sourceware.org (Postfix) with ESMTPS id 92F223858D39 for ; Sat, 6 Aug 2022 23:02:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 92F223858D39 Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-554-baCPu8SGPlyb04GSu_G7gw-1; Sat, 06 Aug 2022 19:02:24 -0400 X-MC-Unique: baCPu8SGPlyb04GSu_G7gw-1 Received: by mail-qk1-f197.google.com with SMTP id q20-20020a05620a0d9400b006b6540e8d79so4996391qkl.14 for ; Sat, 06 Aug 2022 16:02:24 -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=mqnhhw3sc3Ng0EccOoxFmp8Ap4ZJJ0l5B7j1zda2ofI=; b=6WlYONIIFGho23wJr4IEOllWMX4rCjD8QTk0cVDYqqqOhB7tY5BYPzG1yebfK5yaN8 ne+2gjnby0H1/uIBCy+PKysjKoKtjSqO7v730+3PLDZ3fo+uex7LyoFyYityyeCROgsz axXULtkl35p8f6OTquUlIS7cW7AFMgFJY4KHrhrPXWedugUkWNCRDqGjt3277dqgCQeJ +JaZ/1+I5SJYqHvRif99DICEAWNawjBGNzVLFqHDcUlkmuH3Y+oirBndPdRLEWhmUfsx cEf6XL8EDqrYOSieUn5HqGIJI4mfdpXJbQvo5CVWyPLSQbBaQi46lhYDT3f2vCrzkodd mt+w== X-Gm-Message-State: ACgBeo0g2FEYj7UMPRubr8p0lIUup/EP5VDe8az+toUsAQia/PDEgjk6 Nys9inCBb69aXDnGGqR42qHulPFK6jQ0apZMtpzgg9WeIxR8aQzzXpD5J8oWKXBfloVu120yirq mAl+GrihWSFnStWXiXQ== X-Received: by 2002:a05:620a:25c8:b0:6ae:ba71:ea7d with SMTP id y8-20020a05620a25c800b006aeba71ea7dmr9923290qko.547.1659826943422; Sat, 06 Aug 2022 16:02:23 -0700 (PDT) X-Google-Smtp-Source: AA6agR7hGCU9lSfOYE/H8RzRXtOdCQtyxBaazpDE7szjRuz2bd0MvLJlg4Wp9bj+49tUJFyC67hMEQ== X-Received: by 2002:a05:620a:25c8:b0:6ae:ba71:ea7d with SMTP id y8-20020a05620a25c800b006aeba71ea7dmr9923273qko.547.1659826943031; Sat, 06 Aug 2022 16:02:23 -0700 (PDT) Received: from [192.168.44.44] ([72.2.237.35]) by smtp.gmail.com with ESMTPSA id v18-20020a05620a0f1200b006b60f5f53ccsm6255244qkl.25.2022.08.06.16.02.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 06 Aug 2022 16:02:22 -0700 (PDT) Message-ID: <56c01b19-087c-b955-6528-61b4c1fd0ab2@redhat.com> Date: Sat, 6 Aug 2022 16:02:13 -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++: Tweak for -Wpessimizing-move in templates [PR89780] To: Marek Polacek , GCC Patches References: <20220804184642.106756-1-polacek@redhat.com> From: Jason Merrill In-Reply-To: <20220804184642.106756-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.4 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_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-List-Received-Date: Sat, 06 Aug 2022 23:02:28 -0000 On 8/4/22 11:46, Marek Polacek wrote: > In my previous patches I've been extending our std::move warnings, > but this tweak actually dials it down a little bit. As reported in > bug 89780, it's questionable to warn about expressions in templates > that were type-dependent, but aren't anymore because we're instantiating > the template. As in, > > template > Dest withMove() { > T x; > return std::move(x); > } > > template Dest withMove(); // #1 > template Dest withMove(); // #2 > > Saying that the std::move is pessimizing for #1 is not incorrect, but > it's not useful, because removing the std::move would then pessimize #2. > So the user can't really win. At the same time, disabling the warning > just because we're in a template would be going too far, I still want to > warn for > > template > Dest withMove() { > Dest x; > return std::move(x); > } > > because the std::move therein will be pessimizing for any instantiation. > > So I'm using the suppress_warning machinery to that effect. > Problem: I had to add a new group to nowarn_spec_t, otherwise > suppressing the -Wpessimizing-move warning would disable a whole bunch > of other warnings, which we really don't want. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > PR c++/89780 > > gcc/cp/ChangeLog: > > * pt.cc (tsubst_copy_and_build) : Maybe suppress > -Wpessimizing-move. > * typeck.cc (maybe_warn_pessimizing_move): Don't issue warnings > if they are suppressed. > (check_return_expr): Disable -Wpessimizing-move when returning > a dependent expression. > > gcc/ChangeLog: > > * diagnostic-spec.cc (nowarn_spec_t::nowarn_spec_t): Handle > OPT_Wpessimizing_move and OPT_Wredundant_move. > * diagnostic-spec.h (nowarn_spec_t): Add NW_REDUNDANT enumerator. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/Wpessimizing-move3.C: Remove dg-warning. > * g++.dg/cpp0x/Wpessimizing-move7.C: Likewise. > * g++.dg/cpp0x/Wredundant-move2.C: Likewise. > * g++.dg/cpp0x/Wpessimizing-move9.C: New test. > --- > gcc/cp/pt.cc | 3 + > gcc/cp/typeck.cc | 20 +++-- > gcc/diagnostic-spec.cc | 7 +- > gcc/diagnostic-spec.h | 4 +- > .../g++.dg/cpp0x/Wpessimizing-move3.C | 2 +- > .../g++.dg/cpp0x/Wpessimizing-move7.C | 2 +- > .../g++.dg/cpp0x/Wpessimizing-move9.C | 89 +++++++++++++++++++ > gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C | 4 +- > 8 files changed, 119 insertions(+), 12 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index 6c581fe0fb7..fe7e809fc2d 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -21215,6 +21215,9 @@ tsubst_copy_and_build (tree t, > CALL_EXPR_ORDERED_ARGS (call) = ord; > CALL_EXPR_REVERSE_ARGS (call) = rev; > } > + if (warning_suppressed_p (t, OPT_Wpessimizing_move)) > + /* This also suppresses -Wredundant-move. */ > + suppress_warning (ret, OPT_Wpessimizing_move); > } > > RETURN (ret); > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index 2650beb780e..70a5efc45de 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -10430,9 +10430,10 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p) > if (can_do_rvo_p (arg, type)) > { > auto_diagnostic_group d; > - if (warning_at (loc, OPT_Wpessimizing_move, > - "moving a temporary object prevents copy " > - "elision")) > + if (!warning_suppressed_p (expr, OPT_Wpessimizing_move) I don't think we ever want to suppress this warning; moving it to a different warning flag (as I suggested on the other patch) would accomplish that. > + && warning_at (loc, OPT_Wpessimizing_move, > + "moving a temporary object prevents copy " > + "elision")) > inform (loc, "remove % call"); > } > /* The rest of the warnings is only relevant for when we are > @@ -10443,14 +10444,16 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p) > else if (can_do_nrvo_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")) > + if (!warning_suppressed_p (expr, OPT_Wpessimizing_move) > + && warning_at (loc, OPT_Wpessimizing_move, > + "moving a local object in a return statement " > + "prevents copy elision")) > 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 > + && !warning_suppressed_p (expr, OPT_Wredundant_move) > && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true))) > { > /* Make sure that overload resolution would actually succeed > @@ -10695,6 +10698,11 @@ check_return_expr (tree retval, bool *no_warning) > /* We don't know if this is an lvalue or rvalue use, but > either way we can mark it as read. */ > mark_exp_read (retval); > + /* Disable our std::move warnings when we're returning > + a dependent expression (c++/89780). */ > + if (retval && TREE_CODE (retval) == CALL_EXPR) Should this check type_dependent_expression_p? > + /* This also suppresses -Wredundant-move. */ > + suppress_warning (retval, OPT_Wpessimizing_move); > return retval; > } > > diff --git a/gcc/diagnostic-spec.cc b/gcc/diagnostic-spec.cc > index 4341ccfaae9..aece89619e7 100644 > --- a/gcc/diagnostic-spec.cc > +++ b/gcc/diagnostic-spec.cc > @@ -96,7 +96,7 @@ nowarn_spec_t::nowarn_spec_t (opt_code opt) > case OPT_Winit_self: > case OPT_Wuninitialized: > case OPT_Wmaybe_uninitialized: > - m_bits = NW_UNINIT; > + m_bits = NW_UNINIT; > break; > > case OPT_Wdangling_pointer_: > @@ -105,6 +105,11 @@ nowarn_spec_t::nowarn_spec_t (opt_code opt) > m_bits = NW_DANGLING; > break; > > + case OPT_Wpessimizing_move: > + case OPT_Wredundant_move: > + m_bits = NW_REDUNDANT; > + break; > + > default: > /* A catchall group for everything else. */ > m_bits = NW_OTHER; > diff --git a/gcc/diagnostic-spec.h b/gcc/diagnostic-spec.h > index 28e5e5ccc75..e5f1c127d4f 100644 > --- a/gcc/diagnostic-spec.h > +++ b/gcc/diagnostic-spec.h > @@ -45,9 +45,11 @@ public: > NW_DANGLING = 1 << 5, > /* All other unclassified warnings. */ > NW_OTHER = 1 << 6, > + /* Warnings about redundant calls. */ > + NW_REDUNDANT = 1 << 7, > /* All groups of warnings. */ > NW_ALL = (NW_ACCESS | NW_LEXICAL | NW_NONNULL > - | NW_UNINIT | NW_VFLOW | NW_DANGLING | NW_OTHER) > + | NW_UNINIT | NW_VFLOW | NW_DANGLING | NW_REDUNDANT | NW_OTHER) > }; > > nowarn_spec_t (): m_bits () { } > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > index a1af1230b68..c81f29a4ba6 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > @@ -39,7 +39,7 @@ Tp1 > fn2 () > { > Tp2 t; > - return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } > + return std::move (t); > } > > template > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > index a17c7a87b7d..ebbcb2122a6 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > @@ -46,7 +46,7 @@ template > T1 > fn3 () > { > - return std::move (T2{}); // { dg-warning "moving a temporary object prevents copy elision" } > + return std::move (T2{}); As mentioned above, we want to keep this warning; the dangling reference problem is not type-dependent. > } > > void > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C > new file mode 100644 > index 00000000000..9519695eda7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C > @@ -0,0 +1,89 @@ > +// PR c++/89780 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wpessimizing-move -Wredundant-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 Dest { > + Dest() = default; > + Dest(Dest&&); > + Dest(const Dest&); > +}; > +struct Source : Dest {}; > + > +template > +Dest withMove() { > + T x; > + return std::move(x); > +} > + > +template Dest withMove(); > +template Dest withMove(); > + > +template > +Dest bar () { > + return std::move(T()); > +} > + > +template Dest bar(); > +template Dest bar(); > + > +template > +Dest baz (T x) { > + return std::move(x); > +} > + > +void > +call_baz () > +{ > + Dest d; > + Source s; > + baz (d); > + baz (s); > +} > + > +template > +Dest foo () > +{ > + Dest d; > + return std::move(d); // { dg-warning "moving a local object in a return statement prevents copy elision" } > +} > + > +template Dest foo(); > + > +template > +Dest qux () { > + return std::move(Dest()); // { dg-warning "moving a temporary object prevents copy elision" } > +} > + > +template Dest qux(); > + > +template > +Dest qui (Dest x) { > + return std::move(x); // { dg-warning "redundant move in return statement" } > +} > + > +void > +call_qui () > +{ > + Dest d; > + qui (d); > +} > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C > index f181afeeb84..6e0aa4b0277 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C > +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C > @@ -37,14 +37,14 @@ template > Tp1 > fn2 (Tp2 t) > { > - return std::move (t); // { dg-warning "redundant move in return statement" } > + return std::move (t); > } > > template > Tp1 > fn3 (Tp2 t) > { > - return std::move (t); // { dg-warning "redundant move in return statement" } > + return std::move (t); > } > > int > > base-commit: be58bf98e98bb431ed26ca8be84586075fe8be82 > prerequisite-patch-id: f4862bc588ce5fed1d21016fecc4b61feb71eba5 > prerequisite-patch-id: ce490c3c0b991fa7f27315387949c145c66223a4