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

Stabilize unions with `Copy` fields and no destructor #42068

Merged
merged 1 commit into from May 27, 2017

Conversation

@petrochenkov
Copy link
Contributor

petrochenkov commented May 17, 2017

What else is needed:

  • Documentation (rust-lang/reference#57).
  • Making assignments to Copy union fields safe (#42083).
  • Backport? (The "stabilization decision" is from Apr 13, it's just this PR is late.)

cc #32836
r? @nikomatsakis

@steveklabnik
Copy link
Member

steveklabnik commented May 18, 2017

Glad to see this becoming stable!

Backport? (The "stabilization decision" is from Apr 13, it's just this PR is late.)

I am not generally a fan of backporting anything that's not crucial.

@carols10cents
Copy link
Member

carols10cents commented May 22, 2017

ping @nikomatsakis, pinging you on IRC too!

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 23, 2017

I'm waiting on this until #42083 lands.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 24, 2017
…akis

Make assignments to `Copy` union fields safe

This is an accompanying PR to PR rust-lang#42068 stabilizing FFI unions.

This was first proposed in rust-lang#32836 (comment), see subsequent comments as well.
Assignments to `Copy` union fields do not read any data from the union and are [equivalent](rust-lang#32836 (comment)) to whole union assignments, which are safe, so they should be safe as well. This removes a significant number of "false positive" unsafe blocks, in code dealing with FFI unions in particular.

It desirable to make this change now, together with stabilization of FFI unions, because now it affecfts only unstable code, but later it will cause warnings/errors caused by `unused_unsafe` lint in stable code.

cc rust-lang#32836
r? @nikomatsakis
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 24, 2017
…akis

Make assignments to `Copy` union fields safe

This is an accompanying PR to PR rust-lang#42068 stabilizing FFI unions.

This was first proposed in rust-lang#32836 (comment), see subsequent comments as well.
Assignments to `Copy` union fields do not read any data from the union and are [equivalent](rust-lang#32836 (comment)) to whole union assignments, which are safe, so they should be safe as well. This removes a significant number of "false positive" unsafe blocks, in code dealing with FFI unions in particular.

It desirable to make this change now, together with stabilization of FFI unions, because now it affecfts only unstable code, but later it will cause warnings/errors caused by `unused_unsafe` lint in stable code.

cc rust-lang#32836
r? @nikomatsakis
bors added a commit that referenced this pull request May 26, 2017
Make assignments to `Copy` union fields safe

This is an accompanying PR to PR #42068 stabilizing FFI unions.

This was first proposed in #32836 (comment), see subsequent comments as well.
Assignments to `Copy` union fields do not read any data from the union and are [equivalent](#32836 (comment)) to whole union assignments, which are safe, so they should be safe as well. This removes a significant number of "false positive" unsafe blocks, in code dealing with FFI unions in particular.

It desirable to make this change now, together with stabilization of FFI unions, because now it affecfts only unstable code, but later it will cause warnings/errors caused by `unused_unsafe` lint in stable code.

cc #32836
r? @nikomatsakis
@bors
Copy link
Contributor

bors commented May 26, 2017

The latest upstream changes (presumably #42083) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 26, 2017

r=me once rebased

@petrochenkov petrochenkov force-pushed the petrochenkov:ustab branch from b6f463e to 73c73e4 May 26, 2017
@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 26, 2017

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented May 26, 2017

📌 Commit 73c73e4 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented May 26, 2017

Testing commit 73c73e4 with merge fc26e66...

bors added a commit that referenced this pull request May 26, 2017
Stabilize unions with `Copy` fields and no destructor

What else is needed:
- [x] Documentation (rust-lang/reference#57).
- [x] Making assignments to `Copy` union fields safe (#42083).
- [ ] Backport? (The "stabilization decision" is from [Apr 13](#32836 (comment)), it's just this PR is late.)

cc #32836
r? @nikomatsakis
@bors
Copy link
Contributor

bors commented May 27, 2017

💔 Test failed - status-appveyor

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented May 27, 2017

@bors retry

  • #41006 - std.dll spurious removal failure
@bors
Copy link
Contributor

bors commented May 27, 2017

Testing commit 73c73e4 with merge 3e7908f...

bors added a commit that referenced this pull request May 27, 2017
Stabilize unions with `Copy` fields and no destructor

What else is needed:
- [x] Documentation (rust-lang/reference#57).
- [x] Making assignments to `Copy` union fields safe (#42083).
- [ ] Backport? (The "stabilization decision" is from [Apr 13](#32836 (comment)), it's just this PR is late.)

cc #32836
r? @nikomatsakis
@bors
Copy link
Contributor

bors commented May 27, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 3e7908f to master...

@bors bors merged commit 73c73e4 into rust-lang:master May 27, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@petrochenkov petrochenkov mentioned this pull request May 27, 2017
3 of 5 tasks complete
@brson
Copy link
Contributor

brson commented May 31, 2017

Were going to try to backport.

@brson
Copy link
Contributor

brson commented May 31, 2017

cc @rust-lang/compiler

bors added a commit that referenced this pull request May 31, 2017
[beta] Stabilize unions with `Copy` fields and no destructor

- #42068

r? @aturon

cc @petrochenkov
@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 31, 2017

@brson I don't strongly object, but I'm surprised you would backport. I feel like we try to keep backports to regressions or important bug fixes, and only rarely backport stabilizations -- the exception would be if some feature is hotly demanded. But otoh this seems pretty darn low risk, so no real objection.

@brson
Copy link
Contributor

brson commented May 31, 2017

I closed the backport. It was feeling non-trivial and there wasn't a great deal of agreement about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.