-
Notifications
You must be signed in to change notification settings - Fork 70
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
refactor: replace std::io::Error with custom OerError #723
Conversation
0af1137
to
6aaf4be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking great! The major question is to go with the added read_{u{8,16,32},exact}
or just with bytes::Buf
methods and have an oblig length check before deserialization.
I think the single big length check would be preferable, because afterwards the LLVM can probably see that the bytes::Buf
methods cannot panic.
Regarding the failed exchange_rates.rs
test... We might need to disable it, unless there was just some temporary issue.
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
- Introduce `try_read_u8` in `BufOerExt` trait - Remove ByteOrder dep in packet, stream, service-util and ildcp - Add OerError to stream, btp and ccp - stream remove useless error var Signed-off-by: bwty <whalelephant@users.noreply.github.com> Signed-off-by: bwty <whalelephant@users.noreply.github.com>
- added methods in BufOerExt for check and read data into buf - remove bytesorder dependencies - rename previous try_read_u8 to read_u8 Signed-off-by: bwty <whalelephant@users.noreply.github.com>
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
Co-authored-by: Joonas Koivunen <joonas.koivunen@gmail.com> Signed-off-by: bwty <whalelephant@users.noreply.github.com>
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the suggestions (resolved and made them into a PR) I think this is looking good!
Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
Signed-off-by: bwty <whalelephant@users.noreply.github.com>
Oki I understand that the PR is now ready to be merged but since it's already friday lets give this some more time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
I added a couple of subjective comments, so if you don't agree with them you can skip, it is still good to go from me!
Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
the comments on the test case bothered me after reading through them over and over again. Made the cases exhaustive by checking every other short buffer as well. Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
this will allow defining easily constants for more fields. Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
naming: most of the constants seem LEN not LENGTH Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
smaller than the Address::MIN_LEN constant. Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
I hope this doesn't duplicate some existing test ... I did try to look around for it. Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
the string contained a stringified version of chrono::Error, which wasn't much use, see the two cases of changed strings. Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
- more "Invalid Packet" -- these used to be mostly I/O Errors - use display instead of debug formatting Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
Signed-off-by: Joonas Koivunen <joonas.koivunen@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👏 nice job!
Cannot get any reliable perf changes, but at least there's no more IO error, so this is a big win for readability. |
Remove requirement to keep mapping
std::io::Error
into different crates` error type.