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

Implement more traits for std::io::ErrorKind #35911

Merged
merged 2 commits into from
Sep 1, 2016

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Aug 23, 2016

This makes it possible to use it as key in various maps.

This makes it possible to use it as key in various maps.
@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 23, 2016
@alexcrichton
Copy link
Member

Could you expand on the motivation of being able to use in various maps as well? One specific aspect here was to reorder the enum, and it seems like we would want to provide zero guarantees about the ordering of this enum.

@tbu-
Copy link
Contributor Author

tbu- commented Aug 23, 2016

I write a tool that operates on a large number of files, and it tries to display an error summary at the end. Currently, I have an almost-exact mirror of the ErrorKind enum in the code in order to be able to put it into a HashMap.

Do we guarantee that the ordering stays the same for types that derive ordering? I don't think that's all too useful, because we also don't want to guarantee the ordering for other enums where you can obtain it by casting it to an integer (e1 as u32 < e2 as u32). Maybe we need to have some kind of discussion about that.

@Stebalien
Copy link
Contributor

(IMO, these methods should all panic when called on __NonExhaustive as a sanity check but that's probably not too important given that it's unstable).

@ollie27
Copy link
Member

ollie27 commented Aug 23, 2016

If we're not supposed to have any guarantees about the ordering of this enum then it probably shouldn't be a C-Like enum. We're currently exposing an ordering: assert!((ErrorKind::Other as isize) < (ErrorKind::UnexpectedEof as isize)).

@Stebalien
Copy link
Contributor

@ollie27 (that also means that this change cannot reorder fields without being a breaking change).

@tbu-
Copy link
Contributor Author

tbu- commented Aug 24, 2016

@Stebalien I don't think we consider that a breaking change, see e.g. #25246 or rust-lang/rfcs#906. It doesn't realistically break any code.

@sfackler
Copy link
Member

I don't really see why we'd want to reorder these variants?

@tbu-
Copy link
Contributor Author

tbu- commented Aug 24, 2016

Just for consistency I guess, but I can remove that part. I thought Other at the end made the most sense and it was the case until UnexpectedEof was added.

@tbu-
Copy link
Contributor Author

tbu- commented Aug 24, 2016

To clarify, this was ment as a drive-by fix, not part of the whole pull request. I restored the old ordering because apparantly this is not a tiny change.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Aug 29, 2016

📌 Commit c2d064e has been approved by alexcrichton

@sophiajt
Copy link
Contributor

@bors rollup

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 31, 2016
…crichton

Implement more traits for `std::io::ErrorKind`

This makes it possible to use it as key in various maps.
bors added a commit that referenced this pull request Sep 1, 2016
@bors bors merged commit c2d064e into rust-lang:master Sep 1, 2016
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants