Re: [PATCH 1/6] auto_vec copy/move improvements

[Martin Sebor <msebor@gmail.com>]

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.

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.  */
>