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

WIP: rewrite based on MaybeUninit and #[repr(usize)] enum #157

Closed
wants to merge 1 commit into from

Conversation

SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jul 4, 2019

Note: this is very much incomplete.

I’ve been vaguely worried about the unsafe code in this crate for a while, and #124 (comment) prompted me to have another look.

Rust 1.36 adds a MaybeUninit type and deprecates the uninitialized function, because the latter is hard to impossible to use without undefined behavior. So this is a proof-of-concept of what a SmallVec based on MaybeUninit looks like.

This is all new code, rather than trying to retrofit what was there. Mostly for ease of experimentation, but it’s maybe not a bad approach to keep if this is pushed to completion. It’s the opportunity to make some breaking changes to align the API closer to Vec and call it version 1.0.0.

I’ve made an attempt at being more careful with unsafe code, for example with using modules and privacy within the crate to have abstractions that hide some of more tricky unsafety from the rest of the code.

The ideal memory layout for SmallVec (where there isn’t 63 bits wasted because of alignment constraints, next to one bit of enum discriminant) is straightforward to achieve with a union type, but unfortunately those are still unstable for non-Copy fields. I’ve submitted rust-lang/rust#62330 to try and push this along. In the meantime, I’ve managed to find a way to achieve the same layout with #[repr(usize)] on an enum type, but it’s very hacky. Maybe it’s better to wait for proper union types?

I’m submitting this in a PR to avoid throwing it away (it’s not a bad starting point). But I’m going to close this PR to signal that I don’t intend to work a lot more on it in the short term. Still, discussion is welcome in the comment thread.


This change is Reviewable

@SimonSapin SimonSapin closed this Jul 4, 2019
@jdm jdm mentioned this pull request Jul 4, 2019
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