From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com [IPv6:2a00:1450:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id 5F10B3857437 for ; Tue, 15 Jun 2021 06:42:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5F10B3857437 Received: by mail-ed1-x52f.google.com with SMTP id d7so1527001edx.0 for ; Mon, 14 Jun 2021 23:42:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2BH5MHLfg0tOGlW0oGKsex80k/jL6Vb+b0lF0NRtSFs=; b=PrdGA4lKIgG/YS2BWq+wN5RERUXMXLFhchHO3llVwalBSflfEW4bnhBbyna/v63kej K/tT3ofjDQtje7GcM+YDEQYD4qwnVVkU1Q0KQQl5TpgTskU/BoJe/LwCpBExNVSGGA6M 2BHn4CSNUuEvc66om8X6H87oJCn9Icd7P58OadkIT3NLzQMKJzOKUC7utIN78Jbq+SlR bA2vtrzm04qLz/bbIzH4vWEGkN+yF9TLHtBNbRlmGabPdgMOd7K0UStanau07Ua3eJbA 7ABonErIK7OxKdA7rSIMVUslhVW8/cuhGjngPo1MPKWOpmk40t1vAwOxCW/3XtKvo05T YXAA== X-Gm-Message-State: AOAM53133v7Pz1mvqLgTzxKZA5nYJab6HhtPK0vlGAaZMB7mNOLccqo1 atCdE+MofHCFRhM0vKiWq7YWcMIAvpc/GSNAsIY= X-Google-Smtp-Source: ABdhPJzwIJx3xAstNpTeuR1CstKQgA22U2Pu91jpHw+LU/S+VbQLjMKDumr4KdBFedlpFpnMtbanqQ4pY+R7/fNb/gI= X-Received: by 2002:a05:6402:175b:: with SMTP id v27mr21347397edx.61.1623739366813; Mon, 14 Jun 2021 23:42:46 -0700 (PDT) MIME-Version: 1.0 References: <20210615055922.27205-1-tbsaunde@tbsaunde.org> In-Reply-To: <20210615055922.27205-1-tbsaunde@tbsaunde.org> From: Richard Biener Date: Tue, 15 Jun 2021 08:42:35 +0200 Message-ID: Subject: Re: [PATCH 1/6] auto_vec copy/move improvements To: Trevor Saunders Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 15 Jun 2021 06:42:50 -0000 On Tue, Jun 15, 2021 at 8:00 AM Trevor Saunders wrote: > > - Unfortunately using_auto_storage () needs to handle m_vec being null. > - Handle self move of an auto_vec to itself. > - punt on defining copy or move operators for auto_vec with inline storage, > until there is a need for them and we can decide what semantics they should > have. Hmm, that will make using of the CTORs/assignments in "infrastructure" fragile if you consider void foo(vec src) { auto_vec dest (src); ... } bar() { auto_vec a; // vs. auto_vec a.safe_push (X()); // "decays" both to vec foo (a); } that is, it will eventually lead to hard to track down results? I wonder if we should add a m_has_auto_storage and assert that the incoming vector does not instead of just asserting it doesn't use it to make the failure mode at least not dependent so much on "input"? FWIW I agree that we likely want to avoid the copy that would be required when auto-storage is used - OTOH if we can be sure the lifetime of the result cannot be extended beyond the auto-storage provider then copying m_vec will likely just work? Besides this detail the patch looks OK. Thanks, Richard. > - Make sure auto_vec defines the classes move constructor and assignment > operator, as well as ones taking vec, so the compiler does not generate > them for us. Per https://en.cppreference.com/w/cpp/language/move_constructor > the ones taking vec do not count as the classes move constructor or > assignment operator, but we want them as well to assign a plain vec to a > auto_vec. > - Explicitly delete auto_vec's copy constructor and assignment operator. This > prevents unintentional expenssive coppies of the vector and makes it clear > when coppies are needed that that is what is intended. When it is necessary to > copy a vector copy () can be used. > > Signed-off-by: Trevor Saunders > > bootstrapped and regtested on x86_64-linux-gnu, ok? > > gcc/ChangeLog: > > * vec.h (vl_ptr>::using_auto_storage): Handle null m_vec. > (auto_vec::auto_vec): Define move constructor, and delete copy > constructor. > (auto_vec::operator=): Define move assignment and delete copy > assignment. > (auto_vec::auto_vec): Delete copy and move constructors. > (auto_vec::operator=): Delete copy and move assignment. > --- > gcc/vec.h | 41 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 40 insertions(+), 1 deletion(-) > > diff --git a/gcc/vec.h b/gcc/vec.h > index 193377cb69c..ceefa67e1ad 100644 > --- a/gcc/vec.h > +++ b/gcc/vec.h > @@ -1549,6 +1549,16 @@ public: > this->release (); > } > > + // Punt for now on moving auto_vec with inline storage. For now this > + // prevents people creating dangling pointers or the like. > + auto_vec (auto_vec &&) = delete; > + auto_vec &operator= (auto_vec &&) = delete; > + > + // Punt for now on the inline storage, and you probably don't want to copy > + // vectors anyway. If you really must copy a vector use copy (). > + auto_vec(const auto_vec &) = delete; > + auto_vec &operator= (const auto_vec &) = delete; > + > private: > vec m_auto; > T m_data[MAX (N - 1, 1)]; > @@ -1570,14 +1580,43 @@ public: > this->m_vec = r.m_vec; > r.m_vec = NULL; > } > + > + auto_vec (auto_vec &&r) > + { > + gcc_assert (!r.using_auto_storage ()); > + this->m_vec = r.m_vec; > + r.m_vec = NULL; > + } > + > auto_vec& operator= (vec&& r) > { > + if (this == &r) > + return *this; > + > + gcc_assert (!r.using_auto_storage ()); > + this->release (); > + this->m_vec = r.m_vec; > + r.m_vec = NULL; > + return *this; > + } > + > + auto_vec& operator= (auto_vec &&r) > + { > + if (this == &r) > + return *this; > + > gcc_assert (!r.using_auto_storage ()); > this->release (); > this->m_vec = r.m_vec; > r.m_vec = NULL; > return *this; > } > + > + // You probably don't want to copy a vector, so these are deleted to prevent > + // unintentional use. If you really need a copy of the vectors contents you > + // can use copy (). > + auto_vec(const auto_vec &) = delete; > + auto_vec &operator= (const auto_vec &) = delete; > }; > > > @@ -2147,7 +2186,7 @@ template > inline bool > vec::using_auto_storage () const > { > - return m_vec->m_vecpfx.m_using_auto_storage; > + return m_vec ? m_vec->m_vecpfx.m_using_auto_storage : false; > } > > /* Release VEC and call release of all element vectors. */ > -- > 2.20.1 >