Feat:recovereable error functions in scope src/cli #1106
Conversation
vuvoth
changed the title
|
Hi, just to offer some directions in terms of prioritization for a first round of work (this doesn't need to be resolved on a single PR). These are the places that contain the most important parts of the codebase, as far as I can tell:
So starting on those is a better first step 👍🏼 |
There was a problem hiding this comment.
Echoing what @arthurpaulino said, it'll probably be easier if this is done over multiple PRs since there are many places in the codebase that show up in the cargo clippy output.
I'm adding a couple of comments below to try and clarify some points, let me know if you have any questions!
src/z_data/serde/de.rs
Outdated
| let a: [u8; 2] = x.clone().try_into().unwrap(); | ||
| let x_clone = x.clone(); | ||
| let a: [u8; 2] = [x_clone[0], x_clone[1]]; |
There was a problem hiding this comment.
This change doesn't modify how the code works: if x_clone[0] or x_clone[1] is out of bounds, there will be a runtime panic.
An alternative to prevent the panic is to add a match block to handle the result of x.clone().try_into(), and return an Err if it is not Ok. (Similar to the code inside deserialize_u32)
There was a problem hiding this comment.
Thank you! I misunderstood the "match guards" in Rust.
I will fix it soon!
src/z_data/serde/de.rs
Outdated
| // This error can't happend? | ||
| Err(_) => Err(SerdeError::Type( |
There was a problem hiding this comment.
This error could still happen if x.len() is not 8, in which case the .try_into() would fail to convert it into a [u8; 8].
This is because in your change you removed the if x.len() == 8 condition in the match arm (before the =>, see the match guards section of the rust reference for a thorough explanation).
Previously, in both this function (and the deserialize_u32 function that has a similar pattern), it would never run the x.clone().try_into().unwrap(); line unless the if x.len() == 8 condition passed. This means that even though there was an .unwrap() here, it would never panic in practice, since the condition guarantees that the .try_into() should always succeed.
cargo clippy has no way of checking the above conditions, and that's why it listed this location among the rest. (Unfortunately, there's no easy way of checking this besides looking at each .unwrap() call in a case-by-case basis.)
|
Thank @arthurpaulino and @wwared for useful suggestions. I will check it after Lunar New Year. |
vuvoth
force-pushed
the
feat/vu/handle-recovereable-error-in-function
branch
from
d55306a to
1049fbb
Compare
vuvoth
changed the title
vuvoth
changed the title
vuvoth
changed the title
vuvoth
changed the title
There was a problem hiding this comment.
A stylistic remark for the incoming changes of this kind.
I'd usually state this change as a nitpick, but since you're going to make several changes of this kind, it's worth to start with a style that better suits lurk-rs from the beginning.
Also, thank you!
If this is all for what you found in src/cli, it's fine for an initial PR. The other folders can be covered on other PRs
|
Thank you for your comments, @arthurpaulino. It's would be better if I followed the code style you recommend! |
vuvoth
force-pushed
the
feat/vu/handle-recovereable-error-in-function
branch
from
60a5f2a to
d2c0e5f
Compare
|
The current changes look good. I will wait until you set the PR as ready for review before I do another check and run the tests |
* Create a (new) InterpretationData to hold interpretation data on MultiFrame * Remove some unwraps/expects because interpretation data is no longer spread across multiple holders * [unrelated] Undo a change from #1106 where a panic was the desired behavior
* Create a (new) InterpretationData to hold interpretation data on MultiFrame * Remove some unwraps/expects because interpretation data is no longer spread across multiple holders * [unrelated] Undo a change from #1106 where a panic was the desired behavior
Motivation
More context
If this PR look good, I will continue update for another scope like @arthurpaulino suggest in this comment
Thank you for all review and suggestions!