Use thiserror for humility-arch-arm#676
Conversation
mkeeter
force-pushed
the
mkeeter/humility-arch-arm-errors
branch
from
94e5ce8 to
ab11e86
Compare
mkeeter
force-pushed
the
mkeeter/humility-arch-arm-errors
branch
from
ab11e86 to
8e3f290
Compare
jamesmunns
left a comment
jamesmunns
left a comment
There was a problem hiding this comment.
Overall the change looks reasonable, we might want to get in the habit of being a bit more complete in documentation, or at least improve-as-we-go.
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum InstructionError { |
There was a problem hiding this comment.
Do we want to make this #[non_exhaustive] to avoid semver breakage of humility-as-a-lib?
This makes things a little tedious in some places, but we might want to start caring about the impact of semver breaking changes if we expect to start having external consumers.
| #[derive(Debug, thiserror::Error)] | ||
| pub enum InstructionError { | ||
| #[error("multiple source registers")] | ||
| MultipleSourceRegisters, |
There was a problem hiding this comment.
Might be worth including doc comments on the enum, and on each variant, at least answering "what does this error mean, and what should a user do about it if they get it". If I got these errors, I'd probably have to go ping you in chat to figure out what I was doing wrong.
If we expect users to start handling these errors (instead of just bailing), we should probably give them some useful feedback!
| rval | ||
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error)] |
There was a problem hiding this comment.
Oh, we might also want to hit some common derives here as well, PartialEq, Clone, and Hash are usually big ones.
Clone is just generally useful, PartialEq allows for easier asserting (incl in tests), Hash is also one of those "eventually someone wants to stick it in a map" kind of things.
2 participants
This is a tiny PR which gives
humility-arch-armfine-grained error types.