feat: add clear api - #29
Conversation
| return; | ||
| } | ||
|
|
||
| self.deallocate_items(); |
There was a problem hiding this comment.
issue: There's a subtle memory-safety problem hiding in here.
What happens if some drop of T or call to T::TupleRerp::drop_in_place panics inside the clear call?
- Let's assume we've already dropped some T's in the SoAVec (otherwise the problem will not show itself).
- A panic occurs inside of
deallocate_items, meaning that we leave this function without performingset_len(0). - Regardless of if the panic is caught above or goes on to crash the program, the
Dropimpl of our SoAVec will get called: it starts dropping the items anew from the beginning. - Since we already dropped some items in the SoAVec in step 1, step 3 now performs double-free and we are in trouble.
Take a look at how Vec::clear is implemented; we'll need to follow its internal logic to make this safe.
There was a problem hiding this comment.
I tried to look at how Vec::clear worked, but I didn't really understand what would be the equivalent of ptr::drop_in_place(elems).
There was a problem hiding this comment.
Right so; right now we do not have a full equivalent to ptr::drop_in_place. We do have the T::TupleRepr::drop_in_place which works if !T::MUST_DROP_AS_SELF, but for structs that require dropping as Self we simply need to read the data out of the SoAVec, turn it into a T using T::from_tuple, and drop that.
So, effectively it's the full equivalent is deallocate_items except that deallocate_items currently uses self.len(). If we simply take len as a parameter (or perhaps all the ptr, cap, and len values) then we can reuse the function both here and in Drop; in Drop we don't need to call set_len as we're already dropping the SoAVec: if a panic happens inside Drop then it will not be retried.
In clear we'll have to read len, then set_len(0), and then call deallocate_items with the stored len value. That way even if deallocate_items panics, dropping the SoAVec will see a 0 length and will not try to re-drop the items.
There was a problem hiding this comment.
Ok, hopefully I understood. I renamed the function to better match the one used in the actual Vec::clear too.
a604479 to
55ea75f
Compare
Adds a
SoAVec::clear()API.This deallocated all the items inside of the
SoAVecand sets the length to 0.Moved the contents of the
Drop-impl to a function calleddeallocate_itemsso that the same logic is used for clearing/dropping the items and dropping theSoAVec.Closes #20.