From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22c.google.com (mail-oi1-x22c.google.com [IPv6:2607:f8b0:4864:20::22c]) by sourceware.org (Postfix) with ESMTPS id 9FC683857403 for ; Tue, 15 Jun 2021 16:18:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9FC683857403 Received: by mail-oi1-x22c.google.com with SMTP id r16so18384054oiw.3 for ; Tue, 15 Jun 2021 09:18:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=edBHumS04ixyTxx3olVGjHsJ/eNX/V7RHqVzhM8P8HQ=; b=Ci7CGewbcxMiRX7ZMdr4de098+Px4ggU2bHCufu7srdVF+81Mx9xmage9nKnWkBtJk yjquQnZada8UcnUNYwE511aqFtJ80hrFE64PKnKM97qYIseypDo+VqjP1p1M/xMNdshH mWDWn6WGD+lNZI9W5Xk0QJLgFv0oWTx8XcduLQX1kBHqzmy8mZxeiV7t4uqWvWGHphsW TIQcOpNtGKmvbqzWaDKgWC7zIoVvTF8vWbULgsegDaloE35yhaTqVqCNse9V4BuJBMYd DT9AKM56K4jsC7anEVhITw1tfpUep4n31YPEH8aVoig+pjC8H9OH+GpaLDkpC9uaiaXB jKmA== X-Gm-Message-State: AOAM533YCCC+QY+0JqfWd8avyhliMu1D/gsm7KOFeODyeCbkJvcwqm6r oedo/nVLpja7mD923uyTUzqT+mc9LiY= X-Google-Smtp-Source: ABdhPJwPblpiQmZxDizx3B2heNXr0bqeBqc0kBoRg1ws6puK6HKa+LVdABtbjcGtRI3RFvdBM3HiiQ== X-Received: by 2002:aca:fc91:: with SMTP id a139mr3805539oii.12.1623773911744; Tue, 15 Jun 2021 09:18:31 -0700 (PDT) Received: from [192.168.0.41] (184-96-238-78.hlrn.qwest.net. [184.96.238.78]) by smtp.gmail.com with ESMTPSA id s187sm3839656oig.6.2021.06.15.09.18.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 15 Jun 2021 09:18:31 -0700 (PDT) Subject: Re: [PATCH 1/6] auto_vec copy/move improvements To: Trevor Saunders , gcc-patches@gcc.gnu.org References: <20210615055922.27205-1-tbsaunde@tbsaunde.org> From: Martin Sebor Message-ID: <75e9c91d-ce9f-4366-1934-027d1b0784fa@gmail.com> Date: Tue, 15 Jun 2021 10:18:30 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <20210615055922.27205-1-tbsaunde@tbsaunde.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, NICE_REPLY_A, 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 16:18:34 -0000 On 6/14/21 11:59 PM, 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. > - 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. auto_vec should be copy constructible and copy assignable to be usable as its own element (for example). There is nothing to gain by preventing it but make auto_vec harder to use. Martin > --- > 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. */ >