[PATCH 1/6] auto_vec copy/move improvements

Trevor Saunders tbsaunde@tbsaunde.org
Wed Jun 16 03:31:05 GMT 2021


On Tue, Jun 15, 2021 at 10:18:30AM -0600, Martin Sebor wrote:
> 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<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>
> > 
> > bootstrapped and regtested on x86_64-linux-gnu, ok?
> > 
> > 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.
> > 	(auto_vec<T, N>::auto_vec): Delete copy and move constructors.
> > 	(auto_vec<T, N>::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.

I disagree, and I think I've laid out the arguments before so I'll
spare people reading them again.  However here's one simple benefit, I
was curious how many places copy vectors today, in part for other
changes I'd like to make, today I can get that number by simply greping
for "copy ()", compared to the work that would involved to decide if
something will be a move or a copy.  It turns out the answer is about
68, in all of gcc, that's not a lot, and I wonder if making ownership
clearer will let us get rid of some.  Certainly if things change and
someone has a problem to solve where copy () just won't work everything
can be reconsidered, but imho "I have to type .copy ()" is a feature.
Certainly a deleted copy constructor is better than a broken one right?

Thanks

Trev

> 
> 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<T, va_heap, vl_embed> 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<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 +2186,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.  */
> > 
> 


More information about the Gcc-patches mailing list