Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upCombine error types #104
Combine error types #104
Conversation
|
I can recommend what I have done for pub struct Error(Box<ErrorImpl>);
enum ErrorImpl {
/* variants */
}The Box is to drop the size of Error from 48 bytes down to 8 bytes which sped up performance of all of serde_json by 5%, and the newtype wrapper is to let us add or change error variants without a breaking change. Nobody actually pattern-matches on these errors - they just print the message and fail. |
|
Sounds like a good idea. @TyOverby what do you think? |
|
@dtolnay @dgriffen: I'm all for sticking it inside of a Box, but my use-cases for bincode require pattern matching on the error: I really care if the deserialization failed because of a dropped network connection (IoError) or because they sent bad bytes over the wire (programmer error). |
|
I am surprised that allocating is cheaper than moving an extra 40 bytes though. |
|
Oh, I guess if you rarely hit error cases, then it's a pretty good speedup in the happy-path. |
|
Oops, looks like I made a mistake when fixing conflicts. That's the last time I use the Github's conflict resolver. |
|
So how should I move forward with any changes to |
|
@dgriffen: yeah, that sounds like a good move forward! Off the top of my head I can't think of a good enum name though. |
|
|
|
I tried to resolve the merge conflicts with githubs online tool. Let's see if it actually worked! |
|
Looks like you kept the old error types |
|
Well, that's the last time I use that tool. |
|
And here I thought I was helping :| |
|
Looks like a few more merge errors that didnt get caught, I'll clean those up |
|
@dtolnay Want to do another pass? |
| match *self { | ||
| ErrorKind::IoError(ref err) => error::Error::description(err), | ||
| ErrorKind::InvalidEncoding(ref ib) => ib.desc, | ||
| ErrorKind::SequenceMustHaveLength => "bincode can't encode infinite sequences", |
This comment has been minimized.
This comment has been minimized.
dtolnay
Feb 3, 2017
Collaborator
I don't think this error is accurate - the one you have in Display is a better characterization of SequenceMustHaveLength.
| ErrorKind::SequenceMustHaveLength => | ||
| write!(fmt, "Bincode can only encode sequences and maps that have a knowable size ahead of time."), | ||
| ErrorKind::SizeLimit => | ||
| write!(fmt, "SizeLimit"), |
This comment has been minimized.
This comment has been minimized.
dtolnay
Feb 3, 2017
Collaborator
This is the Display representation not the Debug representation, so please make this message self-explanatory.
| fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { | ||
| match *self { | ||
| ErrorKind::IoError(ref ioerr) => | ||
| write!(fmt, "IoError: {}", ioerr), |
This comment has been minimized.
This comment has been minimized.
dtolnay
Feb 3, 2017
Collaborator
Rust identifiers like IoError belong in the Debug representation. For the Display representation I would prefer "IO error: {}".
| ErrorKind::IoError(ref ioerr) => | ||
| write!(fmt, "IoError: {}", ioerr), | ||
| ErrorKind::InvalidEncoding(ref ib) => | ||
| write!(fmt, "InvalidEncoding: {}", ib), |
This comment has been minimized.
This comment has been minimized.
|
The code looks good but the error messages are not as helpful as they could be. Ok to merge and follow up under #98 in another PR. |
|
Looks good! We'll take care of error messages as a part of #98 |
ZoeyR commentedFeb 1, 2017
Closes #100