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.

Sign up for GitHub

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

Jump to bottom

Add Sync to the bounds in io::Error #24133

Merged
merged 1 commit into from
Apr 18, 2015
Merged

Add Sync to the bounds in io::Error #24133

merged 1 commit into from
Apr 18, 2015

Conversation

lilyball
Copy link
Contributor

@lilyball lilyball commented Apr 7, 2015

This allows io::Error values to be stored in Arc properly.

Because this requires Sync of any value passed to io::Error::new()
and modifies the relevant convert::From impls, this is a

[breaking-change]

Fixes #24049.

@lilyball
Add Sync to the bounds in io::Error
9868529 
This allows `io::Error` values to be stored in `Arc` properly.

Because this requires `Sync` of any value passed to `io::Error::new()`
and modifies the relevant `convert::From` impls, this is a

[breaking-change]

Fixes rust-lang#24049.
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@lilyball
Copy link
Contributor Author

lilyball commented Apr 7, 2015

/cc @alexcrichton

This was referenced Apr 7, 2015
Closed
Closed
@nikomatsakis
Copy link
Contributor

r? @alexcrichton

cc @aturon
this

@nikomatsakis
Copy link
Contributor

er, incomplete comment. I was going to say this feels like an API stabilization thing.

@alexcrichton
Copy link
Member

I think that if we go this route that we'll also want to knock out #24135 at the same time as it seems useful to be able to clone io::Error. I'll talk a bit with @aturon today and gauge his thoughts as well. Thanks for this though!

@alexcrichton
Copy link
Member

Ok I chatted with @aturon today about this, and our feeling is that this probably wants to go through an RFC. We couldn't really reach consensus among ourselves and would like to garner some broader opinions. Some specific points we had in mind were:

  • This is a breaking change happening in the beta period, and we currently have not made any breaking changes. There's a lot of discussion on this right now though!
  • It's hard to justify the Sync bound for something other than "so this can be in an Arc"
  • It's unclear how important a Clone implementation is for IoError as there aren't too many use cases for it today. Besides what you're describing, I don't think this has been encountered much elsewhere. Plus you can make an IoError cloneable today by using Rc if really necessary.

So to move forward here I'm going to close this for now. Could you open either an RFC or discuss post on this topic to see how others feel about it as well? Thanks!

@lilyball
Copy link
Contributor Author

Plus you can make an IoError cloneable today by using Rc if really necessary.

Only if I'm ok with my type not being Send. At the moment, my library that wants to clone io::Errors is Send if the underlying Write is Send and it seems a shame to lose that.

@lilyball lilyball mentioned this pull request Apr 12, 2015
Merged
@lilyball
Copy link
Contributor Author

RFC submitted as rust-lang/rfcs#1057

@alexcrichton alexcrichton reopened this Apr 17, 2015
@alexcrichton
Copy link
Member

The RFC has now been merged, so I've reopened this to merge it!

@bors: r+ 9868529

@bors
Copy link
Contributor

bors commented Apr 18, 2015

⌛ Testing commit 9868529 with merge efa6a46...

bors added a commit that referenced this pull request Apr 18, 2015
@bors
Auto merge of #24133 - kballard:add-sync-to-io-error, r=alexcrichton
efa6a46 
This allows `io::Error` values to be stored in `Arc` properly.

Because this requires `Sync` of any value passed to `io::Error::new()`
and modifies the relevant `convert::From` impls, this is a

[breaking-change]

Fixes #24049.
@bors
Copy link
Contributor

bors commented Apr 18, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

@bors bors merged commit 9868529 into rust-lang:master Apr 18, 2015
@alexcrichton alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 23, 2015
@alexcrichton alexcrichton mentioned this pull request Apr 23, 2015
Merged
@pnkfelix pnkfelix added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Apr 23, 2015
@nrc nrc added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 23, 2015
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 23, 2015
@lilyball lilyball deleted the add-sync-to-io-error branch May 15, 2015 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

std::io::Error is Send but not Sync
7 participants
@lilyball @rust-highfive @nikomatsakis @alexcrichton @bors @pnkfelix @nrc