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 AEF223858CDA for ; Sat, 6 Aug 2022 23:08:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AEF223858CDA 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-659-8yR3mnz3NOODYXYj3FgV9A-1; Sat, 06 Aug 2022 19:08:05 -0400 X-MC-Unique: 8yR3mnz3NOODYXYj3FgV9A-1 Received: by mail-qk1-f198.google.com with SMTP id n15-20020a05620a294f00b006b5768a0ed0so4949026qkp.7 for ; Sat, 06 Aug 2022 16:08:05 -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:from:to:references:in-reply-to :content-transfer-encoding; bh=KJhwEq4x5y2XWQ+QNOl5LjB0UzoSizkgzDLWGItHzUM=; b=C9n8QBK2rPE6fUM5ebgo6dxr7BYKVfdN05+qq9495qd2p9eWsz1T8P8b1oIugSmabt kB7udmEVKOutalY1K5EOp3L+cBd1V7F86srxV+3P+9N+rfsylzgJXayP6gyeIwhfUj8f 0UhMRoF1rfpKyEfv2hgkCmsrJENuYr9WDY6Qpbbh9eawTXQ42M+d3k9+PT6anyBM1Ges dS9m3wVfSnJeO6zIX5jsd19xcuHzo4FG2+6UVSBkjbjX3m8DT0fKrSiYaF8SAoIu29cF fd0weeuuRzOWjLFmaI4Sbf104hnNsiHAKXKPHrcBHJiIfLP6tDxuS8IWT6nh9QVXnw3o +ebw== X-Gm-Message-State: ACgBeo0joMDL4qg3ak/8eVM7MFr62LV3jvzyyjUiEgFTjTlYHHzuDicX pm84FQouE65oqyW6Z2pi/S6+zgN6Ja6ui3KteMIONqV93/LkB9Dt1K8o3Uhb42X2cHAFjDRDuOm i4Dlz3yrzNQPvN+Jb/g== X-Received: by 2002:ac8:4e89:0:b0:339:db37:5ce4 with SMTP id 9-20020ac84e89000000b00339db375ce4mr11317087qtp.545.1659827284655; Sat, 06 Aug 2022 16:08:04 -0700 (PDT) X-Google-Smtp-Source: AA6agR6zyTPeHLX7lA1U4wCvqpQg785Nbfb5qV1LNUlhdCa3YE9ovd7N78pWkQdms2J44MloWxTsMQ== X-Received: by 2002:ac8:4e89:0:b0:339:db37:5ce4 with SMTP id 9-20020ac84e89000000b00339db375ce4mr11317072qtp.545.1659827284294; Sat, 06 Aug 2022 16:08:04 -0700 (PDT) Received: from [192.168.44.44] ([72.2.237.35]) by smtp.gmail.com with ESMTPSA id f13-20020ac8464d000000b0031eb215a682sm5017962qto.13.2022.08.06.16.08.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 06 Aug 2022 16:08:03 -0700 (PDT) Message-ID: <242f9ff9-18d8-1da0-f9c5-51615862d22a@redhat.com> Date: Sat, 6 Aug 2022 16:07:54 -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] From: Jason Merrill To: Marek Polacek , GCC Patches References: <20220728001424.396849-1-polacek@redhat.com> <50df8e96-3894-b9d2-dbce-a886c04e9815@redhat.com> In-Reply-To: <50df8e96-3894-b9d2-dbce-a886c04e9815@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: 8bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, BODY_8BITS, 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 23:08:08 -0000 On 8/6/22 15:49, Jason Merrill wrote: > 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. Wait, no, I'm confused, the temporary does live long enough to get copied. I still don't think we want to suppress this warning in the other patch. >> +        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> std::remove_reference<_Tp>::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 >