Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip dropping array elements that don't need drop #802

Merged
merged 1 commit into from
Apr 18, 2020
Merged

Conversation

bluss
Copy link
Member

@bluss bluss commented Apr 18, 2020

While this sounds tautological; to be careful, set the vector length to
zero before dropping the vector of the internal storage of an array.

In some places in ndarray where A: Copy (hence the element does not need drop) we use uninitialized elements in vectors. Setting the length to 0 avoids that the vector tries to drop, slice or otherwise produce values of these elements. (The details of the validity letting this happen with nonzero len, are under discussion as of this writing.)

Related to #796, specifically around details of Array::uninitialized safety.

The argumentation used is that we want the following to be safe:

{
    let a = Array::<bool>::uninitialized(16);
} // array `a` drops at the end of the scope

We want this to be safe even if the array has not been filled - initialized.

Under the hood we have the following operations when the array is created and dropped:

let mut v = Vec::<bool>::with_capacity(16);
v.set_len(16);

drop(v);
// Inside drop we have: {
   drop_in_place(&mut v[..]);
   <drop the actual allocation>
// }

Out of this sequence, only the drop_in_place seems problematic, and setting the vector length to 0 will avoid it. Of course, there aren't complete guarantees about Vec's behavior, but with this change we avoid a problem that has been explicitly flagged. Previously discussed in #685.

While this sounds tautological; to be careful, set the vector length to
zero before dropping the vector of the internal storage of an array.

See the comment in the code for why tihs makes a difference.
@bluss bluss merged commit ea0b69e into master Apr 18, 2020
@bluss bluss deleted the owned-repr branch April 18, 2020 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant