Add BufRead::fill_buf #176
Conversation
| fn from(reader: &'a mut R) -> Self { | ||
| Self { reader } | ||
| } | ||
| } |
There was a problem hiding this comment.
Given there is only a single creator of this future, is this From impl necessary? If it is, could we perhaps add a comment to explain why?
There was a problem hiding this comment.
I wanted to make reader field private, but yeah, it could be an associated function, or just make reader pub(crate).
There was a problem hiding this comment.
@tirr-c This is not a big issue, but given a choice I'd prefer a pub(crate) field and a function in order to not leak any unintended APIs publicly. This impl is currently public in the sense that the user could do .into() to convert a buffer into the future.
| fn from(reader: &'a mut R) -> Self { | ||
| Self { reader } | ||
| } | ||
| } |
There was a problem hiding this comment.
@tirr-c This is not a big issue, but given a choice I'd prefer a pub(crate) field and a function in order to not leak any unintended APIs publicly. This impl is currently public in the sense that the user could do .into() to convert a buffer into the future.
src/io/buf_read/fill_buf.rs
Outdated
| // This is safe because: | ||
| // 1. The buffer is valid for the lifetime of the reader. | ||
| // 2. Output is unrelated to the wrapper (Self). | ||
| result.map_ok(|buf| unsafe { std::mem::transmute::<_, &'a [u8]>(buf) }) |
There was a problem hiding this comment.
I'm always a bit anxious about mixing type inference and transmute, so could you perhaps make types more explicit like this?
| result.map_ok(|buf| unsafe { std::mem::transmute::<_, &'a [u8]>(buf) }) | |
| result.map_ok(|buf| unsafe { std::mem::transmute::<&'_ [u8], &'a [u8]>(buf) }) |
|
A logical follow-up now would be to add the |
|
@stjepang Review addressed! |
|
bors r+ |
176: Add BufRead::fill_buf r=stjepang a=tirr-c Tracking issue: #131. Docs from `std`. **Note** that there is a use of `unsafe` in `fill_buf.rs` that transmutes the lifetime of output buffer. Co-authored-by: Wonwoo Choi <[email protected]>
Build failed
|
ghost
merged commit 
|
This implementation is unsound (this already reverted in edfa235 without explanation, but I leave this comment to prevent someone from accidentally using this implementation, like #763). Specifically, this implementation allows you to write code like this ( let mut r = R([0]);
let mut r = FillBufFuture::new(&mut r);
let mut r = Pin::new(&mut r);
let w = noop_waker();
let mut cx = Context::from_waker(&w);
let x = r.as_mut().poll(&mut cx);
println!("{:?}", x); // Ready(Ok([1]))
let _y = r.as_mut().poll(&mut cx);
println!("{:?}", x); // Ready(Ok([2]))
struct R([u8; 1]);
impl AsyncBufRead for R {
fn poll_fill_buf(mut self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<io::Result<&[u8]>> {
self.0[0] += 1;
Poll::Ready(Ok(&self.into_ref().get_ref().0))
}
fn consume(self: Pin<&mut Self>, _: usize) {
unimplemented!()
}
}EDIT: My previous comments seem incorrect... Probably this needs to make |
Tracking issue: #131. Docs from
std.Note that there is a use of
unsafeinfill_buf.rsthat transmutes the lifetime of output buffer.