Skip to content

/rust

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

Explicitly opt out of `Sync` for `cell` and `mpsc` types #31982

Merged
merged 1 commit into from Mar 2, 2016

Conversation

Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@apasel422 @alexcrichton @reem @bors
@apasel422
Copy link
Member

apasel422 commented Mar 1, 2016

These types were already !Sync, but this improves error messages when they are used in contexts that require Sync, aligning them with conventions used with Rc, among others.

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton Mar 1, 2016

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Mar 1, 2016

Thanks @apasel422! I wonder if it might also be a good idea to add compile-fail tests for these error messages as well? That way we can at least try to ensure the message remains high-quality over time.
src/libstd/sync/mpsc/mod.rs Outdated
@@ -307,6 +310,9 @@ pub struct Iter<'a, T: 'a> {
rx: &'a Receiver<T>
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<'a, T> !Sync for Iter<'a, T> { }

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Mar 1, 2016

Member

Perhaps this and the IntoIter can be left off? They seem less "fundamentally not-Sync" than the other types.
@apasel422
Explicitly opt out of `Sync` for `cell` and `mpsc` types 
These types were already `!Sync`, but this improves error messages when
they are used in contexts that require `Sync`, aligning them with
conventions used with `Rc`, among others.
Loading status checks…
f522d88
@apasel422

This comment has been minimized.

Sign in to view
Copy link
Member Author

apasel422 commented Mar 1, 2016

@alexcrichton Updated.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Mar 2, 2016

@bors: r+ f522d88

Thanks!
@reem

This comment has been minimized.

Sign in to view
Copy link
Contributor

reem commented Mar 2, 2016

This is a great change! The previous error message was pretty bad and a common source of new user confusion.

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 2, 2016

@Manishearth
Rollup merge of rust-lang#31982 - apasel422:sync, r=alexcrichton 
These types were already `!Sync`, but this improves error messages when they are used in contexts that require `Sync`, aligning them with conventions used with `Rc`, among others.

r? @alexcrichton
4ed3826

@Manishearth Manishearth referenced this pull request Mar 2, 2016

Merged

Rollup of 5 pull requests #32001

bors added a commit that referenced this pull request Mar 2, 2016

@bors
Auto merge of #32001 - Manishearth:rollup, r=Manishearth 
- Successful merges: #31919, #31982, #31985, #31989, #31999
- Failed merges:
0400d92

@bors bors merged commit f522d88 into rust-lang:master Mar 2, 2016
1 check was pending

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@apasel422 apasel422 deleted the apasel422:sync branch Mar 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.