Message ID | 20210616031719.29865-1-tbsaunde@tbsaunde.org |
---|---|
State | New |
Headers | show |
Series | [V2] auto_vec copy/move improvements | expand |
On Wed, Jun 16, 2021 at 5:18 AM Trevor Saunders <tbsaunde@tbsaunde.org> wrote: > > - Unfortunately using_auto_storage () needs to handle m_vec being null. > - Handle self move of an auto_vec to itself. > - Make sure auto_vec defines the classes move constructor and assignment > operator, as well as ones taking vec<T>, so the compiler does not generate > them for us. Per https://en.cppreference.com/w/cpp/language/move_constructor > the ones taking vec<T> 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 <tbsaunde@tbsaunde.org> > > This time without the changes to the inline storage version of auto_vec as > requested. bootstrap andregtest on x86_64-linux-gnu with the other patches in > the series ongoing, ok if that passes? OK. Thanks, Richard. > Thanks > > Trev > > gcc/ChangeLog: > > * vec.h (vl_ptr>::using_auto_storage): Handle null m_vec. > (auto_vec<T, 0>::auto_vec): Define move constructor, and delete copy > constructor. > (auto_vec<T, 0>::operator=): Define move assignment and delete copy > assignment. > --- > gcc/vec.h | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/gcc/vec.h b/gcc/vec.h > index 193377cb69c..30ef9a69473 100644 > --- a/gcc/vec.h > +++ b/gcc/vec.h > @@ -1570,14 +1570,43 @@ public: > this->m_vec = r.m_vec; > r.m_vec = NULL; > } > + > + auto_vec (auto_vec<T> &&r) > + { > + gcc_assert (!r.using_auto_storage ()); > + this->m_vec = r.m_vec; > + r.m_vec = NULL; > + } > + > auto_vec& operator= (vec<T, va_heap>&& 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<T> &&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 +2176,7 @@ template<typename T> > inline bool > vec<T, va_heap, vl_ptr>::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 >
On 6/16/21 4:13 AM, Richard Biener via Gcc-patches wrote: > On Wed, Jun 16, 2021 at 5:18 AM Trevor Saunders <tbsaunde@tbsaunde.org> wrote: >> >> - Unfortunately using_auto_storage () needs to handle m_vec being null. >> - Handle self move of an auto_vec to itself. >> - Make sure auto_vec defines the classes move constructor and assignment >> operator, as well as ones taking vec<T>, so the compiler does not generate >> them for us. Per https://en.cppreference.com/w/cpp/language/move_constructor >> the ones taking vec<T> 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 <tbsaunde@tbsaunde.org> >> >> This time without the changes to the inline storage version of auto_vec as >> requested. bootstrap andregtest on x86_64-linux-gnu with the other patches in >> the series ongoing, ok if that passes? > > OK. ... >> + >> + // 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; To reiterate, I strongly disagree with this change as well as with the comment. Martin >> }; >> >> >> @@ -2147,7 +2176,7 @@ template<typename T> >> inline bool >> vec<T, va_heap, vl_ptr>::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 >>
diff --git a/gcc/vec.h b/gcc/vec.h index 193377cb69c..30ef9a69473 100644 --- a/gcc/vec.h +++ b/gcc/vec.h @@ -1570,14 +1570,43 @@ public: this->m_vec = r.m_vec; r.m_vec = NULL; } + + auto_vec (auto_vec<T> &&r) + { + gcc_assert (!r.using_auto_storage ()); + this->m_vec = r.m_vec; + r.m_vec = NULL; + } + auto_vec& operator= (vec<T, va_heap>&& 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<T> &&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 +2176,7 @@ template<typename T> inline bool vec<T, va_heap, vl_ptr>::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. */
- Unfortunately using_auto_storage () needs to handle m_vec being null. - Handle self move of an auto_vec to itself. - Make sure auto_vec defines the classes move constructor and assignment operator, as well as ones taking vec<T>, so the compiler does not generate them for us. Per https://en.cppreference.com/w/cpp/language/move_constructor the ones taking vec<T> 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 <tbsaunde@tbsaunde.org> This time without the changes to the inline storage version of auto_vec as requested. bootstrap andregtest on x86_64-linux-gnu with the other patches in the series ongoing, ok if that passes? Thanks Trev gcc/ChangeLog: * vec.h (vl_ptr>::using_auto_storage): Handle null m_vec. (auto_vec<T, 0>::auto_vec): Define move constructor, and delete copy constructor. (auto_vec<T, 0>::operator=): Define move assignment and delete copy assignment. --- gcc/vec.h | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)