Improve error handling in libflate #23399
Conversation
|
r? @huonw (rust_highfive has picked a reviewer for you, use r? to override) |
| } | ||
| } | ||
|
|
||
| impl Drop for Error { |
There was a problem hiding this comment.
Just so you can't destructure Error in foreign code.
There was a problem hiding this comment.
Ah; I believe the field being private should cover that case adequately?
There was a problem hiding this comment.
You can still do things like let Error(_) = e, removing the possibility of changing this to a struct or an enum later on (or just about anything else than a tuple struct).
There was a problem hiding this comment.
Why is Error defined as a tuple-struct in the first place? I think this is generally not a good idea (#22045). Implementing a pointless Drop seems like a bad solution.
There was a problem hiding this comment.
@nikomatsakis I can't think of a better solution than implementing Drop. Making it a struct means it can't be changed to an enum later on, and vice versa. The tuple struct is not special in that regard. The issue you linked to talks about tuple structs with all-pub fields, this one only has a private field.
EDIT: And I'm not sure why "a pointless Drop" is a bad solution, it has no side effects AFAIK except for not being able to implement Copy, but that should probably not be done anyway.
There was a problem hiding this comment.
This is a private library to rustc where we do not need to worry about backwards compatibility. Additionally breaking let Error(_) = foo does not seem to me to warrant an extra blank impl of Drop. Having this extra impl is unfortunate because it is unexpected and especially without a comment it will be difficult for later readers to understand why this is here.
This removes the error case of the compression functions, the only errors that can occur are incorrect parameters or an out-of-memory condition, both of which are handled with panics in Rust. Also introduces an extensible `Error` type instead of returning an `Option`.
|
@huonw Now without the destructor, and as a proper struct. |
|
@bors r+ |
This removes the error case of the compression functions, the only errors that can occur are incorrect parameters or an out-of-memory condition, both of which are handled with panics in Rust. Also introduces an extensible `Error` type instead of returning an `Option`. The type implements a destructor so you can't destructure it.
This removes the error case of the compression functions, the only errors that can occur are incorrect parameters or an out-of-memory condition, both of which are handled with panics in Rust. Also introduces an extensible `Error` type instead of returning an `Option`. The type implements a destructor so you can't destructure it.
|
💔 Test failed - auto-mac-64-opt |
|
That failure doesn't look related to this pull request. |
|
Passed in the rollup, seems like an intermittent. |
|
Filed #23440 |
This removes the error case of the compression functions, the only errors that
can occur are incorrect parameters or an out-of-memory condition, both of which
are handled with panics in Rust.
Also introduces an extensible
Errortype instead of returning anOption.The type implements a destructor so you can't destructure it.