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 MaybeUninit instead of ManuallyDrop to inhibit the validity invariant of the storage type #3

Merged
merged 4 commits into from
Aug 29, 2019

Conversation

SimonSapin
Copy link
Contributor

This fixes the tests added in the first commit, which failed previously:

assertion failed: Some(InlineArray::<usize, [Box<()>; 2]>::new()).is_some()
assertion failed: Some(RingBuffer::<Box<()>>::new()).is_some()
assertion failed: Some(Chunk::<Box<()>>::new()).is_some()
assertion failed: Some(SparseChunk::<Box<()>>::new()).is_some()

Box<_> contains a pointer that is known not to be null, so Option<Box<_>> can be stored on a single pointer with null representing None. This also works for structs that contain a Box field, even within ManuallyDrop, so writing zero to that memory location is incorrect. With MaybeUninit however, it is correct for any byte of the memory representation to be zero (or not initialized) so the enum layout optimization does not use niches found within MaybeUninit.


Also some less important drive-by changes in the latter two commits, let me know if you’d prefer having them separately or not having them.

```
assertion failed: Some(InlineArray::<usize, [Box<()>; 2]>::new()).is_some()
assertion failed: Some(RingBuffer::<Box<()>>::new()).is_some()
assertion failed: Some(Chunk::<Box<()>>::new()).is_some()
assertion failed: Some(SparseChunk::<Box<()>>::new()).is_some()
```
… calls where straightforward

This also takes care of returning early if needs_drop returns false
It is undecided whether the Rust language should make this UB,
so manipulating raw pointers works either way.
@bodil
Copy link
Owner

bodil commented Aug 29, 2019

This is brilliant, and, more importantly, seems to pass the im-rs stress tests. Thanks!

@bodil bodil merged commit 999d47d into bodil:master Aug 29, 2019
@SimonSapin SimonSapin deleted the uninit branch August 29, 2019 14:47
@SimonSapin
Copy link
Contributor Author

Oops I meant to mention: this raises the minimum Rust version to 1.36

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