Skip to content

/ rust-smallvec

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
Merged

Update union code to use ManuallyDrop #135

merged 1 commit into from Dec 20, 2018

Conversation

@bluss
Copy link
Contributor

bluss commented Dec 19, 2018 •
edited

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
@bluss
Update union code to use ManuallyDrop
Loading status checks…
d6bfa84 
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 bluss:untagged-unions-with-manually-drop branch from e78b4da to d6bfa84 Dec 19, 2018
@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
Contributor

mbrubeck commented Dec 20, 2018

@bors-servo r+

Thanks!
@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2018

📌 Commit d6bfa84 has been approved by mbrubeck
@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2018

⌛ Testing commit d6bfa84 with merge e8999ec...
bors-servo added a commit that referenced this pull request Dec 20, 2018
@bors-servo
Auto merge of #135 - bluss:untagged-unions-with-manually-drop, r=mbru…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
e8999ec 
…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

bors-servo commented Dec 20, 2018

☀️ Test successful - status-travis
Approved by: mbrubeck
Pushing e8999ec to master...
@bors-servo bors-servo merged commit d6bfa84 into servo:master Dec 20, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bluss bluss deleted the bluss:untagged-unions-with-manually-drop branch Dec 22, 2018
@bluss
Copy link
Contributor Author

bluss commented Dec 22, 2018 •
edited

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.
@bluss bluss mentioned this pull request Dec 22, 2018
Closed
mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this pull request Jan 19, 2019
@mbrubeck
Version 0.6.8
4538db6 
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
Merged
bors-servo added a commit that referenced this pull request Jan 19, 2019
@bors-servo
Auto merge of #138 - mbrubeck:bump, r=emilio
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a775b5f 
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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@bluss @mbrubeck @bors-servo
You can’t perform that action at this time.