Re: [09/23] Add a cut-down version of std::span (array_slice)
Richard Biener writes: > On Wed, Aug 10, 2022 at 6:04 PM Martin Jambor wrote: >> >> Hello, >> >> I have one more question/comment about array_slice. Ever since I >> started to use it... >> >> 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. >> > >> > 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.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 ()[N]) : m_base (array), m_size (N) {} >> > + >> > + template >> > + array_slice (const vec ) >> > +: m_base (v.address ()), m_size (v.length ()) {} >> > + >> > + 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 (); >> > + value_type (); >> > + value_type [] (unsigned int i); >> > + >> > + const value_type () const; >> > + const value_type () const; >> > + const value_type [] (unsigned int i) const; >> > + >> > + size_t size () const { return m_size; } >> >> ...this has been a constant source of compile errors, because vectors >> have length () and this is size (). >> >> I understand that the motivation was consistency with std::span, but do >> we really want to add another inconsistency with ourselves? >> >> Given that array_slice is not that much used yet, I believe we can still >> change to be consistent with vectors. I personally think we should but >> at the very least, if we keep it as it is, I'd like us to do so >> deliberately. > > We could alternatively add length in addition to size (and maybe size to > vec<> if std::vector has size but not length) with a comment deprecating > the "non-standard" variant? Yeah, I'd prefer to do the latter: add vec::size as a synonym of vec::length, and deprecate length. Doing anything else seems like it's going to increase the inconsistency rather than decrease it. E.g. we already have uses of (hopefully) uncontroversial standard containers like std::array (my fault). (FWIW, I keep tripping up in the opposite direction: expecting size to be available in vec, like for standard containers.) Thanks, Richard
Re: [09/23] Add a cut-down version of std::span (array_slice)
On Wed, Aug 10, 2022 at 6:04 PM Martin Jambor wrote: > > Hello, > > I have one more question/comment about array_slice. Ever since I > started to use it... > > 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. > > > > 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.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 ()[N]) : m_base (array), m_size (N) {} > > + > > + template > > + array_slice (const vec ) > > +: m_base (v.address ()), m_size (v.length ()) {} > > + > > + 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 (); > > + value_type (); > > + value_type [] (unsigned int i); > > + > > + const value_type () const; > > + const value_type () const; > > + const value_type [] (unsigned int i) const; > > + > > + size_t size () const { return m_size; } > > ...this has been a constant source of compile errors, because vectors > have length () and this is size (). > > I understand that the motivation was consistency with std::span, but do > we really want to add another inconsistency with ourselves? > > Given that array_slice is not that much used yet, I believe we can still > change to be consistent with vectors. I personally think we should but > at the very least, if we keep it as it is, I'd like us to do so > deliberately. We could alternatively add length in addition to size (and maybe size to vec<> if std::vector has size but not length) with a comment deprecating the "non-standard" variant? Richard. > > Thanks, > > Martin >
Re: [09/23] Add a cut-down version of std::span (array_slice)
Hello, I have one more question/comment about array_slice. Ever since I started to use it... 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. > > 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.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 ()[N]) : m_base (array), m_size (N) {} > + > + template > + array_slice (const vec ) > +: m_base (v.address ()), m_size (v.length ()) {} > + > + 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 (); > + value_type (); > + value_type [] (unsigned int i); > + > + const value_type () const; > + const value_type () const; > + const value_type [] (unsigned int i) const; > + > + size_t size () const { return m_size; } ...this has been a constant source of compile errors, because vectors have length () and this is size (). I understand that the motivation was consistency with std::span, but do we really want to add another inconsistency with ourselves? Given that array_slice is not that much used yet, I believe we can still change to be consistent with vectors. I personally think we should but at the very least, if we keep it as it is, I'd like us to do so deliberately. Thanks, Martin
Re: [09/23] Add a cut-down version of std::span (array_slice)
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.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 ()[N]) : m_base (array), m_size (N) {} >> + >> + template >> + array_slice (const vec ) >> +: 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 (); >> + value_type (); >> + value_type [] (unsigned int i); >> + >> + const value_type () const; >> + const value_type () const; >> + const value_type [] (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]; >> +} >> + >>
Re: [09/23] Add a cut-down version of std::span (array_slice)
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.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 ()[N]) : m_base (array), m_size (N) {} > + > + template > + array_slice (const vec ) > +: 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. 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) {} 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 (); > + value_type (); > + value_type [] (unsigned int i); > + > + const value_type () const; > + const value_type () const; > + const value_type [] (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
Re: [09/23] Add a cut-down version of std::span (array_slice)
On 11/13/20 1:15 AM, 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. > > The intention is just to provide what's currently needed. > > gcc/ > * vec.h (array_slice): New class. OK. Obviously we can add more capabilities as we need them. jeff
[09/23] Add a cut-down version of std::span (array_slice)
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. 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.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 ()[N]) : m_base (array), m_size (N) {} + + template + array_slice (const vec ) +: m_base (v.address ()), m_size (v.length ()) {} + + 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 (); + value_type (); + value_type [] (unsigned int i); + + const value_type () const; + const value_type () const; + const value_type [] (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