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

Use NoDrop to fix panic safety issues #6

Merged
merged 3 commits into from
Aug 21, 2015
Merged

Use NoDrop to fix panic safety issues #6

merged 3 commits into from
Aug 21, 2015

Conversation

bluss
Copy link
Owner

@bluss bluss commented Aug 20, 2015

ArrayVec::drop was not panic safe — if there would be a panic during an
element's drop, the discriminant would never be set to Dropped, and the
array elements would potentially double drop.

Fix this by going back to the old NoDrop composition. The NoDrop struct
thas its own Drop impl, that will trigger too on panic during an element's
drop. This serves to make ArrayVec::drop panic safe.

Also tweak IntoIter::drop to make it panic safe: set inner ArrayVec's
length before dropping any elements.

Thank you to @Stebalien for reporting this bug and providing the
excellent testcases in this commit.

Using NoDrop expands ArrayVec to have two drop flags again, but this
is a temporary tradeoff, drop flags will eventually go away.

Fixes #3

ArrayVec::drop was not panic safe — if there would be a panic during an
element's drop, the discriminant would never be set to Dropped, and the
array elements would potentially double drop.

Fix this by going back to the old NoDrop composition. The NoDrop struct
thas its own Drop impl, that will trigger too on panic during an element's
drop. This serves to make ArrayVec::drop panic safe.

Also tweak IntoIter::drop to make it panic safe: set inner ArrayVec's
length before dropping any elements.

Thank you to @Stebalien for reporting this bug and providing the
excellent testcases in this commit.

Using NoDrop expands ArrayVec to have two drop flags again, but this
is a temporary tradeoff, drop flags will eventually go away.

Fixes #3
@bluss bluss mentioned this pull request Aug 20, 2015
@Stebalien
Copy link

LGTM

@bluss
Copy link
Owner Author

bluss commented Aug 20, 2015

Using feature nodrop/no_drop_flag (requires nightly) eliminates the NoDrop drop flag, and should recover the space again.

@bluss
Copy link
Owner Author

bluss commented Aug 20, 2015

Thank you for helping with this!

bluss added a commit that referenced this pull request Aug 21, 2015
Use NoDrop to fix panic safety issues
@bluss bluss merged commit 20500c5 into master Aug 21, 2015
@bluss bluss deleted the nodrop-restored branch August 21, 2015 12:56
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.

2 participants