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 <[email protected]>
> >
> > 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. */
> >
>
