| Message ID | 20210615055922.27205-1-tbsaunde@tbsaunde.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] auto_vec copy/move improvements | expand |
On Tue, Jun 15, 2021 at 8:00 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. > - 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. Hmm, that will make using of the CTORs/assignments in "infrastructure" fragile if you consider void foo(vec<T> src) { auto_vec<T> dest (src); ... } bar() { auto_vec<X> a; // vs. auto_vec<X, 1> a.safe_push (X()); // "decays" both to vec<X> foo (a); } that is, it will eventually lead to hard to track down results? I wonder if we should add a m_has_auto_storage and assert that the incoming vector does not instead of just asserting it doesn't use it to make the failure mode at least not dependent so much on "input"? FWIW I agree that we likely want to avoid the copy that would be required when auto-storage is used - OTOH if we can be sure the lifetime of the result cannot be extended beyond the auto-storage provider then copying m_vec will likely just work? Besides this detail the patch looks OK. Thanks, Richard. > - 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. > --- > 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. */ > -- > 2.20.1 >
On Tue, Jun 15, 2021 at 08:42:35AM +0200, Richard Biener wrote: > On Tue, Jun 15, 2021 at 8:00 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. > > - 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. > > Hmm, that will make using of the CTORs/assignments in "infrastructure" > fragile if you consider It definitely restricts what you can do with auto_vec with inline storage. However that restriction is preexisting, and this just turns it into a assertion failure rather than memory corruption. So its definitely not the final answer, but its better than what we have today I believe, and leaves options open for when this has a user, as this bootstraps nothing needs it today. > void foo(vec<T> src) > { > auto_vec<T> dest (src); > ... > } > > bar() > { > auto_vec<X> a; // vs. auto_vec<X, 1> > a.safe_push (X()); // "decays" both to vec<X> > foo (a); > } > > that is, it will eventually lead to hard to track down results? I wonder if we > should add a m_has_auto_storage and assert that the incoming vector > does not instead of just asserting it doesn't use it to make the failure mode > at least not dependent so much on "input"? I'm not sure I follow this part. I think example you are thinking of is something like this void foo(auto_vec<x> &&src) { auto_vec<x> dst(src); ... } And then some caller who wants to use inline storage void bar() { auto-vec<x> a; a.safe_push (x ()); foo (a); } Which today I believe ends up with dst containing a pointer to part of a, which is bogus and probably going to lead to memory corruption. After this patch we get an assert when we try and create dst because src.using_auto_storage () is true. That's certainly not ideal, but better than what we have today. > FWIW I agree that we likely want to avoid the copy that would be required > when auto-storage is used - OTOH if we can be sure the lifetime of the > result cannot be extended beyond the auto-storage provider then copying > m_vec will likely just work? If I understand the case your thinking of correctly my question would be why are you making a copy at all then, rather than passing a pointer or reference to the original vector? I would think the two cases where a copy may make sense is when the new object outlives the source, or when you wish to mutate the new object leaving the original one unchanged, for either of those copying the m_vec pointer so it points into the original object wouldn't work? > Besides this detail the patch looks OK. I think there's some risk of shooting yourself in the foot with the inline storage version as it is today, but I'd be ok with spliting that part out into a separate patch and only adjusting the version with no inline storage here. I believe that's enough for the rest of the series to work properly. Thanks! Trev > > Thanks, > Richard. > > > - 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. > > --- > > 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. */ > > -- > > 2.20.1 > >
On Tue, Jun 15, 2021 at 9:04 AM Trevor Saunders <tbsaunde@tbsaunde.org> wrote: > > On Tue, Jun 15, 2021 at 08:42:35AM +0200, Richard Biener wrote: > > On Tue, Jun 15, 2021 at 8:00 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. > > > - 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. > > > > Hmm, that will make using of the CTORs/assignments in "infrastructure" > > fragile if you consider > > It definitely restricts what you can do with auto_vec with inline > storage. However that restriction is preexisting, and this just turns > it into a assertion failure rather than memory corruption. You mean the CTOR from vec<> is auto-generated at the moment? > So its > definitely not the final answer, but its better than what we have today > I believe, and leaves options open for when this has a user, as this > bootstraps nothing needs it today. > > > void foo(vec<T> src) > > { > > auto_vec<T> dest (src); > > ... > > } > > > > bar() > > { > > auto_vec<X> a; // vs. auto_vec<X, 1> > > a.safe_push (X()); // "decays" both to vec<X> > > foo (a); > > } > > > > that is, it will eventually lead to hard to track down results? I wonder if we > > should add a m_has_auto_storage and assert that the incoming vector > > does not instead of just asserting it doesn't use it to make the failure mode > > at least not dependent so much on "input"? > > I'm not sure I follow this part. I think example you are thinking of is > something like this > void foo(auto_vec<x> &&src) > { > auto_vec<x> dst(src); > ... > } > > And then some caller who wants to use inline storage > void bar() > { > auto-vec<x> a; > a.safe_push (x ()); > foo (a); > } > > Which today I believe ends up with dst containing a pointer to part of > a, which is bogus and probably going to lead to memory corruption. > After this patch we get an assert when we try and create dst because > src.using_auto_storage () is true. That's certainly not ideal, but > better than what we have today. OK, so I guess one useful way to use the CTOR is when transfering vector ownership to a function, but I expected that void foo (auto_vec<x> mine) { } would already do the trick here, destroying 'mine' when foo exits? > > FWIW I agree that we likely want to avoid the copy that would be required > > when auto-storage is used - OTOH if we can be sure the lifetime of the > > result cannot be extended beyond the auto-storage provider then copying > > m_vec will likely just work? > > If I understand the case your thinking of correctly my question would be > why are you making a copy at all then, rather than passing a pointer or > reference to the original vector? I would think the two cases where a > copy may make sense is when the new object outlives the source, or when > you wish to mutate the new object leaving the original one unchanged, > for either of those copying the m_vec pointer so it points into the > original object wouldn't work? vec<> is used as (const) "reference" in a lot of places, avoiding the extra indirection that happens when using const vec<> & since passing its sole pointer member is cheap. (maybe const vec<> should be passed in all those cases though) > > Besides this detail the patch looks OK. > > I think there's some risk of shooting yourself in the foot with the > inline storage version as it is today, but I'd be ok with spliting that > part out into a separate patch and only adjusting the version with no > inline storage here. I believe that's enough for the rest of the series > to work properly. I trust you with the change but I'm not too familiar with C++ to trust myself with a final OK, so if you can split out this part and post it separately that would make me more comfortable. Thanks, Richard. > > Thanks! > > Trev > > > > > Thanks, > > Richard. > > > > > - 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. > > > --- > > > 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. */ > > > -- > > > 2.20.1 > > >
On Tue, Jun 15, 2021 at 09:11:52AM +0200, Richard Biener wrote: > On Tue, Jun 15, 2021 at 9:04 AM Trevor Saunders <tbsaunde@tbsaunde.org> wrote: > > > > On Tue, Jun 15, 2021 at 08:42:35AM +0200, Richard Biener wrote: > > > On Tue, Jun 15, 2021 at 8:00 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. > > > > - 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. > > > > > > Hmm, that will make using of the CTORs/assignments in "infrastructure" > > > fragile if you consider > > > > It definitely restricts what you can do with auto_vec with inline > > storage. However that restriction is preexisting, and this just turns > > it into a assertion failure rather than memory corruption. > > You mean the CTOR from vec<> is auto-generated at the moment? I actually had to test this to be sure. It depends, the constructor to copy auto_vec<T, N> to auto_vec<T, N> is generated and just coppies the representation, its basically memcpy, so that doesn't work properly. If you attempt to copy auto_vec<T, P> to auto_vec<T, Q> the compiler will already refuse to generate the necessary constructor since the fields don't match up, and so the case where the amount of inline storage is different already fails to compile. > > So its > > definitely not the final answer, but its better than what we have today > > I believe, and leaves options open for when this has a user, as this > > bootstraps nothing needs it today. > > > > > void foo(vec<T> src) > > > { > > > auto_vec<T> dest (src); > > > ... > > > } > > > > > > bar() > > > { > > > auto_vec<X> a; // vs. auto_vec<X, 1> > > > a.safe_push (X()); // "decays" both to vec<X> > > > foo (a); > > > } > > > > > > that is, it will eventually lead to hard to track down results? I wonder if we > > > should add a m_has_auto_storage and assert that the incoming vector > > > does not instead of just asserting it doesn't use it to make the failure mode > > > at least not dependent so much on "input"? > > > > I'm not sure I follow this part. I think example you are thinking of is > > something like this > > void foo(auto_vec<x> &&src) > > { > > auto_vec<x> dst(src); > > ... > > } > > > > And then some caller who wants to use inline storage > > void bar() > > { > > auto-vec<x> a; > > a.safe_push (x ()); > > foo (a); > > } > > > > Which today I believe ends up with dst containing a pointer to part of > > a, which is bogus and probably going to lead to memory corruption. > > After this patch we get an assert when we try and create dst because > > src.using_auto_storage () is true. That's certainly not ideal, but > > better than what we have today. > > OK, so I guess one useful way to use the CTOR is when transfering vector > ownership to a function, but I expected that > > void foo (auto_vec<x> mine) > { > } > > would already do the trick here, destroying 'mine' when foo exits? Yes, after this patch you will have to move the vector into the argument of the function with something like foo (move (freeby));. Today that doesn't work because you get an implicitly generated copy constructor for auto_vec<T> that just coppies the pointer producing an auto_vec in the callie and the caller that both think they own the same vector. > > > FWIW I agree that we likely want to avoid the copy that would be required > > > when auto-storage is used - OTOH if we can be sure the lifetime of the > > > result cannot be extended beyond the auto-storage provider then copying > > > m_vec will likely just work? > > > > If I understand the case your thinking of correctly my question would be > > why are you making a copy at all then, rather than passing a pointer or > > reference to the original vector? I would think the two cases where a > > copy may make sense is when the new object outlives the source, or when > > you wish to mutate the new object leaving the original one unchanged, > > for either of those copying the m_vec pointer so it points into the > > original object wouldn't work? > > vec<> is used as (const) "reference" in a lot of places, avoiding the > extra indirection that happens when using const vec<> & since passing > its sole pointer member is cheap. (maybe const vec<> should be passed > in all those cases though) Certainly the C++ way of doing things is to pass const vec<T> & and hope the abstraction gets optimized out. Its unfortunate, but its also not really clear how else you'd mark that your giving the called function a possibly const borrowed view of the vector. I suppose you can have yet another type that you use in arguments and removes the layer of abstraction when the called function doesn't need to mutate the vector. Certainly if the call can mutate the vector you need to, and already do, pass a vec<>& or vec<>*, and its just the case of functions that only read the vector where this optimization can apply. I find myself wondering how much this manual optimization matters, or if at this point lto bootstrapping production compilers takes care of this for us? That's a question I don't think I have the data to answer. > > > Besides this detail the patch looks OK. > > > > I think there's some risk of shooting yourself in the foot with the > > inline storage version as it is today, but I'd be ok with spliting that > > part out into a separate patch and only adjusting the version with no > > inline storage here. I believe that's enough for the rest of the series > > to work properly. > > I trust you with the change but I'm not too familiar with C++ to > trust myself with a final OK, so if you can split out this part and > post it separately that would make me more comfortable. Sure, just to be clear you mean the deleted constructors and assignment operators for the version of auto_vec<> with inline storage? I'll split that out and make sure the series still bootstrapps and tests ok without it, I really think it should but might as well confirm. Trev > > Thanks, > Richard. > > > > > Thanks! > > > > Trev > > > > > > > > Thanks, > > > Richard. > > > > > > > - 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. > > > > --- > > > > 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. */ > > > > -- > > > > 2.20.1 > > > >
On Tue, Jun 15, 2021 at 9:57 AM Trevor Saunders <tbsaunde@tbsaunde.org> wrote: > > On Tue, Jun 15, 2021 at 09:11:52AM +0200, Richard Biener wrote: > > On Tue, Jun 15, 2021 at 9:04 AM Trevor Saunders <tbsaunde@tbsaunde.org> wrote: > > > > > > On Tue, Jun 15, 2021 at 08:42:35AM +0200, Richard Biener wrote: > > > > On Tue, Jun 15, 2021 at 8:00 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. > > > > > - 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. > > > > > > > > Hmm, that will make using of the CTORs/assignments in "infrastructure" > > > > fragile if you consider > > > > > > It definitely restricts what you can do with auto_vec with inline > > > storage. However that restriction is preexisting, and this just turns > > > it into a assertion failure rather than memory corruption. > > > > You mean the CTOR from vec<> is auto-generated at the moment? > > I actually had to test this to be sure. It depends, the constructor to > copy auto_vec<T, N> to auto_vec<T, N> is generated and just coppies the > representation, its basically memcpy, so that doesn't work properly. If > you attempt to copy auto_vec<T, P> to auto_vec<T, Q> the compiler will > already refuse to generate the necessary constructor since the fields > don't match up, and so the case where the amount of inline storage is > different already fails to compile. > > > > So its > > > definitely not the final answer, but its better than what we have today > > > I believe, and leaves options open for when this has a user, as this > > > bootstraps nothing needs it today. > > > > > > > void foo(vec<T> src) > > > > { > > > > auto_vec<T> dest (src); > > > > ... > > > > } > > > > > > > > bar() > > > > { > > > > auto_vec<X> a; // vs. auto_vec<X, 1> > > > > a.safe_push (X()); // "decays" both to vec<X> > > > > foo (a); > > > > } > > > > > > > > that is, it will eventually lead to hard to track down results? I wonder if we > > > > should add a m_has_auto_storage and assert that the incoming vector > > > > does not instead of just asserting it doesn't use it to make the failure mode > > > > at least not dependent so much on "input"? > > > > > > I'm not sure I follow this part. I think example you are thinking of is > > > something like this > > > void foo(auto_vec<x> &&src) > > > { > > > auto_vec<x> dst(src); > > > ... > > > } > > > > > > And then some caller who wants to use inline storage > > > void bar() > > > { > > > auto-vec<x> a; > > > a.safe_push (x ()); > > > foo (a); > > > } > > > > > > Which today I believe ends up with dst containing a pointer to part of > > > a, which is bogus and probably going to lead to memory corruption. > > > After this patch we get an assert when we try and create dst because > > > src.using_auto_storage () is true. That's certainly not ideal, but > > > better than what we have today. > > > > OK, so I guess one useful way to use the CTOR is when transfering vector > > ownership to a function, but I expected that > > > > void foo (auto_vec<x> mine) > > { > > } > > > > would already do the trick here, destroying 'mine' when foo exits? > > Yes, after this patch you will have to move the vector into the argument > of the function with something like foo (move (freeby));. Today that > doesn't work because you get an implicitly generated copy constructor > for auto_vec<T> that just coppies the pointer producing an auto_vec in > the callie and the caller that both think they own the same vector. > > > > > FWIW I agree that we likely want to avoid the copy that would be required > > > > when auto-storage is used - OTOH if we can be sure the lifetime of the > > > > result cannot be extended beyond the auto-storage provider then copying > > > > m_vec will likely just work? > > > > > > If I understand the case your thinking of correctly my question would be > > > why are you making a copy at all then, rather than passing a pointer or > > > reference to the original vector? I would think the two cases where a > > > copy may make sense is when the new object outlives the source, or when > > > you wish to mutate the new object leaving the original one unchanged, > > > for either of those copying the m_vec pointer so it points into the > > > original object wouldn't work? > > > > vec<> is used as (const) "reference" in a lot of places, avoiding the > > extra indirection that happens when using const vec<> & since passing > > its sole pointer member is cheap. (maybe const vec<> should be passed > > in all those cases though) > > Certainly the C++ way of doing things is to pass const vec<T> & and hope > the abstraction gets optimized out. Its unfortunate, but its also not > really clear how else you'd mark that your giving the called function a > possibly const borrowed view of the vector. I suppose you can have yet > another type that you use in arguments and removes the layer of > abstraction when the called function doesn't need to mutate the vector. > Certainly if the call can mutate the vector you need to, and already do, > pass a vec<>& or vec<>*, and its just the case of functions that only > read the vector where this optimization can apply. I find myself > wondering how much this manual optimization matters, or if at this point > lto bootstrapping production compilers takes care of this for us? That's > a question I don't think I have the data to answer. > > > > > Besides this detail the patch looks OK. > > > > > > I think there's some risk of shooting yourself in the foot with the > > > inline storage version as it is today, but I'd be ok with spliting that > > > part out into a separate patch and only adjusting the version with no > > > inline storage here. I believe that's enough for the rest of the series > > > to work properly. > > > > I trust you with the change but I'm not too familiar with C++ to > > trust myself with a final OK, so if you can split out this part and > > post it separately that would make me more comfortable. > > Sure, just to be clear you mean the deleted constructors and assignment > operators for the version of auto_vec<> with inline storage? Yes. > I'll split > that out and make sure the series still bootstrapps and tests ok without > it, I really think it should but might as well confirm. Thanks, Richard. > Trev > > > > > Thanks, > > Richard. > > > > > > > > Thanks! > > > > > > Trev > > > > > > > > > > > Thanks, > > > > Richard. > > > > > > > > > - 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. > > > > > --- > > > > > 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. */ > > > > > -- > > > > > 2.20.1 > > > > >
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. */ >
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. */ > > >
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. */
- 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. --- gcc/vec.h | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)