Add Cursor type #717
Add Cursor type #717
Conversation
This is a no_std variant of `std::io::Cursor`. Fixes rust-embedded#631. The implementation was generated by an LLM by copying the std impl and adapting it. I (Zeeshan) reviewed the code and adjusted small bits. The tests added are further guarantee that the code is doing what it's supposed to do.
| /// The standard library implements some I/O traits on various types which | ||
| /// are commonly used as a buffer, like `Cursor<Vec<u8>>` and `Cursor<&[u8]>`. | ||
| /// | ||
| /// This is the `embedded-io` equivalent of [`std::io::Cursor`]. |
There was a problem hiding this comment.
Maybe this should mention that this also took big parts of the implementation from std?
I'm not sure which amount of attribution is legally required by the licenses, but I think it's at least good style to disclose such copying.
There was a problem hiding this comment.
it's at least good style to disclose such copying.
- If it's complicated code and/or it's a specific author(s) to attribute, then I could agree but it's not really the case here.
- In any case, I don't think this should be in user-facing documentation as it doesn't affect users in any way. I'm OK with adding a non-doc comment that says so.
- Is this not true for any other parts of this crate? If it is the case, then we should mention it there too, no? 🤔
There was a problem hiding this comment.
It's probably more a personal preference than a rule, and I have no idea if other parts of this crate have been copied from elsewhere. (And my wording "it's at least good style to ..." is likely too strong, I should have written something like "I'd prefer to").
And I agree that it shouldn't be in the user-facing documentation. A simple comment "Parts of this file were copied from std::io::Cursor" would do.
If no one else comments on it, just take it as my personal opinion and feel free to ignore my comment.
There was a problem hiding this comment.
Just an opinion: A reason to comment copied code is if it turns out to have a bug the person troubleshooting can check if the original code has already been fixed.
There was a problem hiding this comment.
Sure, I'll add the comment. Thank you for your detailed feedback. 👍
Any comments on the changes themselves? I'd like this to be merged soon.
There was a problem hiding this comment.
Sure, I'll ask the comment. Thank you for your detailed feedback. 👍
Any comments on the changes themselves? I'd like this to be merged soon.
|
@spookyvision may I ask what makes you 👎 this PR? If it's the use of LLM, perhaps you would have felt fine about this if I hadn't followed the guidelines of the Rust embedded community and hadn't disclosed the use of LLM? |
| /// let mut cursor = Cursor::new(&mut buf[..]); | ||
| /// | ||
| /// cursor.write_all(b"some bytes").unwrap(); | ||
| /// // Internal buffer is not at capacity so writing more bytes fails. |
There was a problem hiding this comment.
I don't think this comment makes sense. Was it meant to be "Internal buffer is now at capacity"?
You also don't describe how it fails.
Does it return a Err? Which one? The std version will keep calling write until it's all written, so this difference in behaviour should be documented.
| } | ||
| } | ||
|
|
||
| // Write implementation for Cursor<&mut [u8]>. |
There was a problem hiding this comment.
Why does this comment exist? It's just restating what the code says in about the same number of words.
Also none of the other impl blocks above got a comment so it's pretty weird that you decided to document this one.
|
|
||
| use super::{cmp, Cursor}; | ||
|
|
||
| // Write implementation for Cursor<Vec<u8>>. |
There was a problem hiding this comment.
This comment provides negative value (it's just the function signature).
| } | ||
| } | ||
|
|
||
| // Write implementation for Cursor<&mut Vec<u8>>. |
There was a problem hiding this comment.
This comment provides negative value (it's just the function signature).
|
|
||
| use super::{cmp, Cursor, CursorError}; | ||
|
|
||
| // Write implementation for Cursor<Box<[u8]>>. |
There was a problem hiding this comment.
This comment provides negative value (it's just the function signature).
| /// Returns the remaining slice. | ||
| #[inline] | ||
| pub fn remaining_slice(&self) -> &[u8] { |
There was a problem hiding this comment.
In std they removed remaining_slice and is_empty and replaced them with split and split_mut.
I think switching to those functions would be an improvement (since remaining_slice is equal to Cursor.split().1) and is_empty is equal to Cursor.split().1.is_empty(), but unlike these functions it would not be unambiguous what it means for both readers and writers.
| #[inline] | ||
| pub fn is_empty(&self) -> bool { | ||
| self.remaining_slice().is_empty() | ||
| } |
There was a problem hiding this comment.
As above, this is Cursor.split().1.is_empty() on std.
| if amt == 1 { | ||
| buf[0] = slice[0]; | ||
| } else { | ||
| buf[..amt].copy_from_slice(&slice[..amt]); | ||
| } |
There was a problem hiding this comment.
Is it worth having an optimisation for a read of length 1 here?
Have you checked that this produces better code in the general case?
| if !buf.is_empty() && len == 0 { | ||
| return Err(CursorError::Full); | ||
| } |
There was a problem hiding this comment.
On std this would return Ok(0), does it make sense to deviate from that behaviour here?
| if !buf.is_empty() && amt == 0 { | ||
| return Err(CursorError::Full); | ||
| } |
There was a problem hiding this comment.
On std this would return Ok(0), does it make sense to deviate from that behaviour here?
5 participants
This is a no_std variant of
std::io::Cursor.Fixes #631.
The implementation was generated by an LLM by copying the std impl and adapting it. I (Zeeshan) reviewed the code and adjusted small bits. The tests added are a further assurance that the code is doing what it's supposed to do. Of course, it needs to be still reviewed by at least another person before we can be 100% certain about it.