-
Notifications
You must be signed in to change notification settings - Fork 278
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
Vendor std::io::Cursor as buf::Cursor (closes #75) #146
Conversation
Extracts the minimal parts of std::io::Cursor needed to support bytes use cases. This is needed to support no_std as std::io is unavailable in these contexts. It also (arguably) improves the API ergonomics by simplifying imports. Per notes on tokio-rs#75, changes the u64 position to a u32 to save space.
pub fn set_position(&mut self, pos: u32) { self.pos = pos; } | ||
} | ||
|
||
impl<T> Read for Cursor<T> where T: AsRef<[u8]> { |
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.
The Read
and BufRead
impls here aren't actually used by bytes
at all. I have included them as without them the following example fails:
That said, perhaps people are expecting them? (and possibly Write
as well?)
@@ -15,8 +15,7 @@ use std::{cmp, io, ptr}; | |||
/// The simplest `Buf` is a `Cursor` wrapping a `[u8]`. | |||
/// | |||
/// ``` | |||
/// use bytes::Buf; | |||
/// use std::io::Cursor; | |||
/// use bytes::{Buf, Cursor}; |
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 a nice ergonomic improvement over:
use bytes::Buf;
use std::io::Cursor;
#[derive(Clone, Debug)] | ||
pub struct Cursor<T> { | ||
inner: T, | ||
pos: u32, |
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.
This is a u64
in std::io
, however @alexcrichton suggested using a smaller type here: #75 (comment)
Note this is a breaking API change which means it can't be landed until bytes 0.5. |
In the technical sense this is unfortunately still a breaking change because Whether it's actually breaking in a practical sense is a different question, though, which I'm not sure about. |
Yeah, duly noted this is a breaking change. I'm also uncertain it's the best path forward. |
After talking with @carllerche it sounds like if a breaking change is to happen here, it would probably be better to get rid of cursors altogether. |
Extracts the minimal parts of std::io::Cursor needed to support bytes use cases.
This is needed to support no_std as std::io is unavailable in these contexts.
It also (arguably) improves the API ergonomics by simplifying imports.
Per notes on #75, changes the u64 position to a u32 to save space.