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

Update union code to use ManuallyDrop #135

Merged
merged 1 commit into from Dec 20, 2018

Conversation

bluss
Copy link
Contributor

@bluss bluss commented Dec 19, 2018

At the time of this writing, this breaking change is proposed, see rust-lang/rust/pull/56440.
Smallvec would have to be updated before the change can be merged upstream. Thanks!

A Rust breaking change to the untagged_unions feature means that no
union fields may have destructors, so we need use ManuallyDrop also
around the inline union field.


This change is Reviewable

A Rust breaking change to the untagged_unions feature, means that no
union fields may have destructors, so we need use ManuallyDrop also
around the inline union field.
@bluss bluss force-pushed the untagged-unions-with-manually-drop branch from e78b4da to d6bfa84 Compare December 19, 2018 21:15
@bluss
Copy link
Contributor Author

bluss commented Dec 19, 2018

This change takes us further on the road to stable unions for SmallVec

@mbrubeck
Copy link
Collaborator

@bors-servo r+

Thanks!

@bors-servo
Copy link
Contributor

📌 Commit d6bfa84 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

⌛ Testing commit d6bfa84 with merge e8999ec...

bors-servo pushed a commit that referenced this pull request Dec 20, 2018
…beck

Update union code to use ManuallyDrop

At the time of this writing, this breaking change is *proposed*, see rust-lang/rust/pull/56440.
Smallvec would have to be updated before the change can be merged upstream. Thanks!

A Rust breaking change to the untagged_unions feature means that no
union fields may have destructors, so we need use ManuallyDrop also
around the inline union field.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/135)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: mbrubeck
Pushing e8999ec to master...

@bors-servo bors-servo merged commit d6bfa84 into servo:master Dec 20, 2018
@bluss bluss deleted the untagged-unions-with-manually-drop branch December 22, 2018 08:33
@bluss
Copy link
Contributor Author

bluss commented Dec 22, 2018

I've reviewed the surrounding code, and it's a bit hard to talk about soundness with the unsafe code guidelines making a rumbling noise in the background, but without clear pronouncements.

One place of worry about this PR could be if there is a practical difference of uninitialized value inside ManuallyDrop inside a union vs just the previous uninitialized value inside a union. Since this is nightly only code, we could “upgrade” to MaybeUninit. (Edit: this specific concern dismissed @ rust-lang/rust#56440 (comment))

With MaybeUninit the question remains about partial initialization and the soundness of forming &A references when the A value is partially initialized, and if there is a way to do partial initialization correctly with raw pointers.

mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this pull request Jan 19, 2019
Change log:

* Don't leak memory if an iterator panics during `extend` (servo#137)
* Update the unstable `union` feature for better forward compatibility (servo#135)
@mbrubeck mbrubeck mentioned this pull request Jan 19, 2019
bors-servo pushed a commit that referenced this pull request Jan 19, 2019
Version 0.6.8

Change log:

* Don't leak memory if an iterator panics during `extend` (#137)
* Update the unstable `union` feature for better forward compatibility (#135)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/138)
<!-- Reviewable:end -->
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.

None yet

3 participants