From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 968283858C39 for ; Wed, 3 Aug 2022 15:31:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 968283858C39 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D7CB511FB; Wed, 3 Aug 2022 08:31:53 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.37]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id AC7FF3F70D; Wed, 3 Aug 2022 08:31:52 -0700 (PDT) From: Richard Sandiford To: Martin Jambor Mail-Followup-To: Martin Jambor , gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org Subject: Re: [09/23] Add a cut-down version of std::span (array_slice) References: Date: Wed, 03 Aug 2022 16:31:51 +0100 In-Reply-To: (Martin Jambor's message of "Wed, 03 Aug 2022 17:13:28 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-52.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_NONE, SPF_NONE, TXREP 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: Wed, 03 Aug 2022 15:31:55 -0000 Martin Jambor writes: > Hi Richard, > > On Fri, Nov 13 2020, Richard Sandiford via Gcc-patches wrote: >> A later patch wants to be able to pass around subarray views of an >> existing array. The standard class to do that is std::span, but it's >> a C++20 thing. This patch just adds a cut-down version of it. > > thanks a lot for introducing it. I hope to use it as a unified view > into something which might be a GC vec or heap vec an an auto_vec. > > But I have one question: > >> >> The intention is just to provide what's currently needed. >> >> gcc/ >> * vec.h (array_slice): New class. >> --- >> gcc/vec.h | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 120 insertions(+) >> >> diff --git a/gcc/vec.h b/gcc/vec.h >> index f02beddc975..7768de9f518 100644 >> --- a/gcc/vec.h >> +++ b/gcc/vec.h >> @@ -2128,6 +2128,126 @@ release_vec_vec (vec > &vec) >> vec.release (); >> } >> >> +// Provide a subset of the std::span functionality. (We can't use std::span >> +// itself because it's a C++20 feature.) >> +// >> +// In addition, provide an invalid value that is distinct from all valid >> +// sequences (including the empty sequence). This can be used to return >> +// failure without having to use std::optional. >> +// >> +// There is no operator bool because it would be ambiguous whether it is >> +// testing for a valid value or an empty sequence. >> +template >> +class array_slice >> +{ >> + template friend class array_slice; >> + >> +public: >> + using value_type = T; >> + using iterator = T *; >> + using const_iterator = const T *; >> + >> + array_slice () : m_base (nullptr), m_size (0) {} >> + >> + template >> + array_slice (array_slice other) >> + : m_base (other.m_base), m_size (other.m_size) {} >> + >> + array_slice (iterator base, unsigned int size) >> + : m_base (base), m_size (size) {} >> + >> + template >> + array_slice (T (&array)[N]) : m_base (array), m_size (N) {} >> + >> + template >> + array_slice (const vec &v) >> + : m_base (v.address ()), m_size (v.length ()) {} >> + > > What is the reason for making the parameter const here? > > The problem is that if you do for example: > > auto_vec test_base; > test_base.quick_grow_cleared (10); > array_slice test(test_base); > > the constructor will get a const reference to test_base and so will > invoke the const variant of v.address() which returns a const bool * > which cannot be assigned into non-const qualified base. AFAICS, the > constructor only works if the array_slice is array_slice. > > Is that intentional? I am not a C++ expert and can be easily > overlooking something. I understand that users need to be careful not > to cause reallocation of the underlying vector while the array_slice > exists but the const qualifier does not achieve that. (A wild idea to > be to add a array_slice ref-counter to auto_vec, which seems to be less > space-efficiency-critical than other vecs, and assert on reallocation > when it is not zero, hehe). > > Removing the const qualifier in the constructor parameter makes the > error go away - as does adding another constructor without it, which > might be the correct thing to do. Yeah, the latter sounds better to me. (The existing uses of array_slice are for const elements, which is why I didn't come across this.) > On a related note, would the following constructor be a good addition to > the class (I can make it const too)? > > template > array_slice (vec *v) > : m_base (v ? v->address () : nullptr), m_size (v ? v->length (): 0) {} LGTM. Thanks, Richard > Thanks, > > Martin > > > >> + iterator begin () { return m_base; } >> + iterator end () { return m_base + m_size; } >> + >> + const_iterator begin () const { return m_base; } >> + const_iterator end () const { return m_base + m_size; } >> + >> + value_type &front (); >> + value_type &back (); >> + value_type &operator[] (unsigned int i); >> + >> + const value_type &front () const; >> + const value_type &back () const; >> + const value_type &operator[] (unsigned int i) const; >> + >> + size_t size () const { return m_size; } >> + size_t size_bytes () const { return m_size * sizeof (T); } >> + bool empty () const { return m_size == 0; } >> + >> + // An invalid array_slice that represents a failed operation. This is >> + // distinct from an empty slice, which is a valid result in some contexts. >> + static array_slice invalid () { return { nullptr, ~0U }; } >> + >> + // True if the array is valid, false if it is an array like INVALID. >> + bool is_valid () const { return m_base || m_size == 0; } >> + >> +private: >> + iterator m_base; >> + unsigned int m_size; >> +}; >> + >> +template >> +inline typename array_slice::value_type & >> +array_slice::front () >> +{ >> + gcc_checking_assert (m_size); >> + return m_base[0]; >> +} >> + >> +template >> +inline const typename array_slice::value_type & >> +array_slice::front () const >> +{ >> + gcc_checking_assert (m_size); >> + return m_base[0]; >> +} >> + >> +template >> +inline typename array_slice::value_type & >> +array_slice::back () >> +{ >> + gcc_checking_assert (m_size); >> + return m_base[m_size - 1]; >> +} >> + >> +template >> +inline const typename array_slice::value_type & >> +array_slice::back () const >> +{ >> + gcc_checking_assert (m_size); >> + return m_base[m_size - 1]; >> +} >> + >> +template >> +inline typename array_slice::value_type & >> +array_slice::operator[] (unsigned int i) >> +{ >> + gcc_checking_assert (i < m_size); >> + return m_base[i]; >> +} >> + >> +template >> +inline const typename array_slice::value_type & >> +array_slice::operator[] (unsigned int i) const >> +{ >> + gcc_checking_assert (i < m_size); >> + return m_base[i]; >> +} >> + >> +template >> +array_slice >> +make_array_slice (T *base, unsigned int size) >> +{ >> + return array_slice (base, size); >> +} >> + >> #if (GCC_VERSION >= 3000) >> # pragma GCC poison m_vec m_vecpfx m_vecdata >> #endif >> -- >> 2.17.1