Conversation
qwandor
force-pushed
the
memory
branch
2 times, most recently
from
4380a93 to
53e9692
Compare
qwandor
force-pushed
the
memory
branch
2 times, most recently
from
5a1eb08 to
98cb828
Compare
m4tx
left a comment
m4tx
left a comment
There was a problem hiding this comment.
Looks good overall, but please fix the minor issues I've pointed out.
| // `ref_from_bytes` shouldn't panic because each chunk will always be a multiple | ||
| // of 4 bytes because of `chunks_exact`. | ||
| #[allow(clippy::unwrap_used)] |
There was a problem hiding this comment.
If we have a specific reason anyway, can't we just use .expect() instead with the comment's content in the reason message? Or at least add reason = "..." to the attribute?
On the side, #[expect] should almost always be used instead of #[allow] so that we'll get a warning when the lint is not triggered (for instance when the code below changes and one forgets to remove the #[allow] attribute).
| } | ||
|
|
||
| /// The value of an `initial-mapped-area` property. | ||
| #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] |
There was a problem hiding this comment.
| #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] | |
| #[derive(Clone, Copy, Debug, Default, Eq, PartialEq, Hash)] |
nitpick: there's some small chance someone will want to put this in a hash set.
| /// | ||
| /// Returns an error if a property's name or value cannot be read, or the | ||
| /// size of the value isn't a multiple of 5 cells. | ||
| #[allow(clippy::missing_panics_doc)] |
There was a problem hiding this comment.
Please use #[expect] for this (and I believe this belongs right before the line that causes this.
| // try_into can't return an error, because we passed a chunk size matching what | ||
| // `InitialMappedArea::from_cells` expects. | ||
| #[allow(clippy::unwrap_used)] | ||
| Some( | ||
| property | ||
| .as_prop_encoded_array(5)? | ||
| .map(|chunk| InitialMappedArea::from_cells(chunk.try_into().unwrap())), | ||
| ) |
There was a problem hiding this comment.
Same as above: does it make sense to use .expect(<message>)/#[expect(clippy::unwrap_used, reason = "..."] for this?
| // `ref_from_bytes` shouldn't panic because each chunk will always be a multiple | ||
| // of 4 bytes because of `chunks_exact`. | ||
| #[allow(clippy::unwrap_used)] |
| // try_into can't return an error, because we passed a chunk size matching what | ||
| // `InitialMappedArea::from_cells` expects. | ||
| #[allow(clippy::unwrap_used)] | ||
| Some( | ||
| property | ||
| .as_prop_encoded_array(5)? | ||
| .map(|chunk| InitialMappedArea::from_cells(chunk.try_into().unwrap())), | ||
| ) |
| /// | ||
| /// Returns an error if a property's name or value cannot be read, or the | ||
| /// size of the value isn't a multiple of 5 cells. | ||
| #[allow(clippy::missing_panics_doc)] |
| } | ||
|
|
||
| /// The value of an `initial-mapped-area` property. | ||
| #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] |
2 participants
No description provided.