feat(io): implement Read, ReadReady, BufRead, Write, and WriteReady for VecDeque<u8>
#697
Conversation
This is the same MSRV as embedded-io, which embedded-io-async depends on.
mkroening
force-pushed
the
io-vec_deque
branch
from
300c8b1 to
e72721f
Compare
mkroening
force-pushed
the
io-vec_deque
branch
from
38683cb to
ad6a7d1
Compare
mkroening
changed the title
Read, BufRead, and Write for VecDeque<u8>Read, ReadReady, BufRead, Write, and WriteReady for VecDeque<u8>
mkroening
changed the title
Read, ReadReady, BufRead, Write, and WriteReady for VecDeque<u8>Read, ReadReady, BufRead, Write, and WriteReady for VecDeque<u8>
There was a problem hiding this comment.
Generally I think it's good to have those provided, and the code does that.
The commits look a bit messy – was the intention to split them up between the sync and the async part?
The code looks good to me; several comments, but some are more for documenting that a part was checked (just mark them as resolved if you don't want to add anything). Those where I do suggest changes are all minor, so feel free to change things or not.
(Also, I'm just a guest contributor here, just trying to make it easier to get this merged).
| } | ||
|
|
||
| #[inline] | ||
| async fn read_exact(&mut self, buf: &mut [u8]) -> Result<(), ReadExactError<Self::Error>> { |
There was a problem hiding this comment.
This looks like it's more code than the read above combined with the provided read_exactly – is there really a point to spelling this out over letting the compiler assemble the provided one?
There was a problem hiding this comment.
Apparently, yes. I did not reevaluate this code coming from std, but it was a conscious decision to specialize it: rust-lang/rust#132039
| @@ -0,0 +1,83 @@ | |||
| //! Adapted from std. | |||
There was a problem hiding this comment.
It's pretty verbatim in places. I think that's fine given that both are licensed under "MIT OR Apache-2.0", and the original files do not have any copyright notes that point out concrete contributors (so the embedded-hal copyright notice should already cover that).
| } | ||
|
|
||
| #[inline] | ||
| async fn flush(&mut self) -> Result<(), Self::Error> { |
There was a problem hiding this comment.
This is redundant with the provided method.
|
Thanks for the external review! :)
No, the intention was to separate concerns:
|
|
Thanks; then all is fine with me. |
This PR implements both sync and async versions of
Read,BufRead, andWrite, as well asReadReadyandWriteReadyforVecDeque<u8>by adapting the corresponding implementations fromstd.