Address reviewer comments

Signed-off-by: Nick Cameron <nrc@ncameron.org>
This commit is contained in:
Nick Cameron 2022-08-11 15:52:29 +01:00
parent 1a2122fff0
commit ac70aea985
18 changed files with 74 additions and 55 deletions

View File

@ -703,7 +703,7 @@ impl Read for File {
self.inner.read_vectored(bufs) self.inner.read_vectored(bufs)
} }
fn read_buf(&mut self, cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { fn read_buf(&mut self, cursor: BorrowedCursor<'_>) -> io::Result<()> {
self.inner.read_buf(cursor) self.inner.read_buf(cursor)
} }
@ -755,7 +755,7 @@ impl Read for &File {
self.inner.read(buf) self.inner.read(buf)
} }
fn read_buf(&mut self, cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { fn read_buf(&mut self, cursor: BorrowedCursor<'_>) -> io::Result<()> {
self.inner.read_buf(cursor) self.inner.read_buf(cursor)
} }

View File

@ -266,7 +266,7 @@ impl<R: Read> Read for BufReader<R> {
Ok(nread) Ok(nread)
} }
fn read_buf(&mut self, mut cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { fn read_buf(&mut self, mut cursor: BorrowedCursor<'_>) -> io::Result<()> {
// If we don't have any buffered data and we're doing a massive read // If we don't have any buffered data and we're doing a massive read
// (larger than our internal buffer), bypass our internal buffer // (larger than our internal buffer), bypass our internal buffer
// entirely. // entirely.
@ -278,7 +278,7 @@ impl<R: Read> Read for BufReader<R> {
let prev = cursor.written(); let prev = cursor.written();
let mut rem = self.fill_buf()?; let mut rem = self.fill_buf()?;
rem.read_buf(cursor.clone())?; rem.read_buf(cursor.reborrow())?;
self.consume(cursor.written() - prev); //slice impl of read_buf known to never unfill buf self.consume(cursor.written() - prev); //slice impl of read_buf known to never unfill buf

View File

@ -93,7 +93,7 @@ impl Buffer {
if self.pos >= self.filled { if self.pos >= self.filled {
debug_assert!(self.pos == self.filled); debug_assert!(self.pos == self.filled);
let mut buf: BorrowedBuf<'_> = (&mut *self.buf).into(); let mut buf = BorrowedBuf::from(&mut *self.buf);
// SAFETY: `self.filled` bytes will always have been initialized. // SAFETY: `self.filled` bytes will always have been initialized.
unsafe { unsafe {
buf.set_init(self.filled); buf.set_init(self.filled);

View File

@ -106,7 +106,7 @@ impl<I: Write> BufferedCopySpec for BufWriter<I> {
if read_buf.capacity() >= DEFAULT_BUF_SIZE { if read_buf.capacity() >= DEFAULT_BUF_SIZE {
let mut cursor = read_buf.unfilled(); let mut cursor = read_buf.unfilled();
match reader.read_buf(cursor.clone()) { match reader.read_buf(cursor.reborrow()) {
Ok(()) => { Ok(()) => {
let bytes_read = cursor.written(); let bytes_read = cursor.written();

View File

@ -323,10 +323,10 @@ where
Ok(n) Ok(n)
} }
fn read_buf(&mut self, mut cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { fn read_buf(&mut self, mut cursor: BorrowedCursor<'_>) -> io::Result<()> {
let prev_written = cursor.written(); let prev_written = cursor.written();
Read::read_buf(&mut self.fill_buf()?, cursor.clone())?; Read::read_buf(&mut self.fill_buf()?, cursor.reborrow())?;
self.pos += (cursor.written() - prev_written) as u64; self.pos += (cursor.written() - prev_written) as u64;

View File

@ -21,7 +21,7 @@ impl<R: Read + ?Sized> Read for &mut R {
} }
#[inline] #[inline]
fn read_buf(&mut self, cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { fn read_buf(&mut self, cursor: BorrowedCursor<'_>) -> io::Result<()> {
(**self).read_buf(cursor) (**self).read_buf(cursor)
} }
@ -125,7 +125,7 @@ impl<R: Read + ?Sized> Read for Box<R> {
} }
#[inline] #[inline]
fn read_buf(&mut self, cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { fn read_buf(&mut self, cursor: BorrowedCursor<'_>) -> io::Result<()> {
(**self).read_buf(cursor) (**self).read_buf(cursor)
} }
@ -249,7 +249,7 @@ impl Read for &[u8] {
} }
#[inline] #[inline]
fn read_buf(&mut self, mut cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { fn read_buf(&mut self, mut cursor: BorrowedCursor<'_>) -> io::Result<()> {
let amt = cmp::min(cursor.capacity(), self.len()); let amt = cmp::min(cursor.capacity(), self.len());
let (a, b) = self.split_at(amt); let (a, b) = self.split_at(amt);
@ -427,7 +427,7 @@ impl<A: Allocator> Read for VecDeque<u8, A> {
} }
#[inline] #[inline]
fn read_buf(&mut self, cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { fn read_buf(&mut self, cursor: BorrowedCursor<'_>) -> io::Result<()> {
let (ref mut front, _) = self.as_slices(); let (ref mut front, _) = self.as_slices();
let n = cmp::min(cursor.capacity(), front.len()); let n = cmp::min(cursor.capacity(), front.len());
Read::read_buf(front, cursor)?; Read::read_buf(front, cursor)?;

View File

@ -370,7 +370,7 @@ pub(crate) fn default_read_to_end<R: Read + ?Sized>(r: &mut R, buf: &mut Vec<u8>
} }
let mut cursor = read_buf.unfilled(); let mut cursor = read_buf.unfilled();
match r.read_buf(cursor.clone()) { match r.read_buf(cursor.reborrow()) {
Ok(()) => {} Ok(()) => {}
Err(e) if e.kind() == ErrorKind::Interrupted => continue, Err(e) if e.kind() == ErrorKind::Interrupted => continue,
Err(e) => return Err(e), Err(e) => return Err(e),
@ -462,7 +462,7 @@ pub(crate) fn default_read_exact<R: Read + ?Sized>(this: &mut R, mut buf: &mut [
} }
} }
pub(crate) fn default_read_buf<F>(read: F, mut cursor: BorrowedCursor<'_, '_>) -> Result<()> pub(crate) fn default_read_buf<F>(read: F, mut cursor: BorrowedCursor<'_>) -> Result<()>
where where
F: FnOnce(&mut [u8]) -> Result<usize>, F: FnOnce(&mut [u8]) -> Result<usize>,
{ {
@ -812,7 +812,7 @@ pub trait Read {
/// ///
/// The default implementation delegates to `read`. /// The default implementation delegates to `read`.
#[unstable(feature = "read_buf", issue = "78485")] #[unstable(feature = "read_buf", issue = "78485")]
fn read_buf(&mut self, buf: BorrowedCursor<'_, '_>) -> Result<()> { fn read_buf(&mut self, buf: BorrowedCursor<'_>) -> Result<()> {
default_read_buf(|b| self.read(b), buf) default_read_buf(|b| self.read(b), buf)
} }
@ -821,10 +821,10 @@ pub trait Read {
/// This is equivalent to the [`read_exact`](Read::read_exact) method, except that it is passed a [`BorrowedCursor`] rather than `[u8]` to /// This is equivalent to the [`read_exact`](Read::read_exact) method, except that it is passed a [`BorrowedCursor`] rather than `[u8]` to
/// allow use with uninitialized buffers. /// allow use with uninitialized buffers.
#[unstable(feature = "read_buf", issue = "78485")] #[unstable(feature = "read_buf", issue = "78485")]
fn read_buf_exact(&mut self, mut cursor: BorrowedCursor<'_, '_>) -> Result<()> { fn read_buf_exact(&mut self, mut cursor: BorrowedCursor<'_>) -> Result<()> {
while cursor.capacity() > 0 { while cursor.capacity() > 0 {
let prev_written = cursor.written(); let prev_written = cursor.written();
match self.read_buf(cursor.clone()) { match self.read_buf(cursor.reborrow()) {
Ok(()) => {} Ok(()) => {}
Err(e) if e.kind() == ErrorKind::Interrupted => continue, Err(e) if e.kind() == ErrorKind::Interrupted => continue,
Err(e) => return Err(e), Err(e) => return Err(e),
@ -2586,7 +2586,7 @@ impl<T: Read> Read for Take<T> {
Ok(n) Ok(n)
} }
fn read_buf(&mut self, mut buf: BorrowedCursor<'_, '_>) -> Result<()> { fn read_buf(&mut self, mut buf: BorrowedCursor<'_>) -> Result<()> {
// Don't call into inner reader at all at EOF because it may still block // Don't call into inner reader at all at EOF because it may still block
if self.limit == 0 { if self.limit == 0 {
return Ok(()); return Ok(());
@ -2609,7 +2609,7 @@ impl<T: Read> Read for Take<T> {
} }
let mut cursor = sliced_buf.unfilled(); let mut cursor = sliced_buf.unfilled();
self.inner.read_buf(cursor.clone())?; self.inner.read_buf(cursor.reborrow())?;
let new_init = cursor.init_ref().len(); let new_init = cursor.init_ref().len();
let filled = sliced_buf.len(); let filled = sliced_buf.len();
@ -2626,7 +2626,7 @@ impl<T: Read> Read for Take<T> {
self.limit -= filled as u64; self.limit -= filled as u64;
} else { } else {
let written = buf.written(); let written = buf.written();
self.inner.read_buf(buf.clone())?; self.inner.read_buf(buf.reborrow())?;
self.limit -= (buf.written() - written) as u64; self.limit -= (buf.written() - written) as u64;
} }

View File

@ -6,7 +6,7 @@ mod tests;
use crate::cmp; use crate::cmp;
use crate::fmt::{self, Debug, Formatter}; use crate::fmt::{self, Debug, Formatter};
use crate::io::{Result, Write}; use crate::io::{Result, Write};
use crate::mem::MaybeUninit; use crate::mem::{self, MaybeUninit};
/// A borrowed byte buffer which is incrementally filled and initialized. /// A borrowed byte buffer which is incrementally filled and initialized.
/// ///
@ -23,9 +23,9 @@ use crate::mem::MaybeUninit;
/// ``` /// ```
/// ///
/// A `BorrowedBuf` is created around some existing data (or capacity for data) via a unique reference /// A `BorrowedBuf` is created around some existing data (or capacity for data) via a unique reference
/// (`&mut`). The `BorrowedBuf` can be configured (e.g., using `clear` or `set_init`), but otherwise /// (`&mut`). The `BorrowedBuf` can be configured (e.g., using `clear` or `set_init`), but cannot be
/// is read-only. To write into the buffer, use `unfilled` to create a `BorrowedCursor`. The cursor /// directly written. To write into the buffer, use `unfilled` to create a `BorrowedCursor`. The cursor
/// has write-only access to the unfilled portion of the buffer (you can think of it like a /// has write-only access to the unfilled portion of the buffer (you can think of it as a
/// write-only iterator). /// write-only iterator).
/// ///
/// The lifetime `'data` is a bound on the lifetime of the underlying data. /// The lifetime `'data` is a bound on the lifetime of the underlying data.
@ -55,7 +55,7 @@ impl<'data> From<&'data mut [u8]> for BorrowedBuf<'data> {
let len = slice.len(); let len = slice.len();
BorrowedBuf { BorrowedBuf {
//SAFETY: initialized data never becoming uninitialized is an invariant of BorrowedBuf // SAFETY: initialized data never becoming uninitialized is an invariant of BorrowedBuf
buf: unsafe { (slice as *mut [u8]).as_uninit_slice_mut().unwrap() }, buf: unsafe { (slice as *mut [u8]).as_uninit_slice_mut().unwrap() },
filled: 0, filled: 0,
init: len, init: len,
@ -95,14 +95,21 @@ impl<'data> BorrowedBuf<'data> {
/// Returns a shared reference to the filled portion of the buffer. /// Returns a shared reference to the filled portion of the buffer.
#[inline] #[inline]
pub fn filled(&self) -> &[u8] { pub fn filled(&self) -> &[u8] {
//SAFETY: We only slice the filled part of the buffer, which is always valid // SAFETY: We only slice the filled part of the buffer, which is always valid
unsafe { MaybeUninit::slice_assume_init_ref(&self.buf[0..self.filled]) } unsafe { MaybeUninit::slice_assume_init_ref(&self.buf[0..self.filled]) }
} }
/// Returns a cursor over the unfilled part of the buffer. /// Returns a cursor over the unfilled part of the buffer.
#[inline] #[inline]
pub fn unfilled<'this>(&'this mut self) -> BorrowedCursor<'this, 'data> { pub fn unfilled<'this>(&'this mut self) -> BorrowedCursor<'this> {
BorrowedCursor { start: self.filled, buf: self } BorrowedCursor {
start: self.filled,
// SAFETY: we never assign into `BorrowedCursor::buf`, so treating its
// lifetime covariantly is safe.
buf: unsafe {
mem::transmute::<&'this mut BorrowedBuf<'data>, &'this mut BorrowedBuf<'this>>(self)
},
}
} }
/// Clears the buffer, resetting the filled region to empty. /// Clears the buffer, resetting the filled region to empty.
@ -141,25 +148,37 @@ impl<'data> BorrowedBuf<'data> {
/// `BorrowedBuf` and can no longer be accessed or re-written by the cursor. I.e., the cursor tracks /// `BorrowedBuf` and can no longer be accessed or re-written by the cursor. I.e., the cursor tracks
/// the unfilled part of the underlying `BorrowedBuf`. /// the unfilled part of the underlying `BorrowedBuf`.
/// ///
/// The `'buf` lifetime is a bound on the lifetime of the underlying buffer. `'data` is a bound on /// The lifetime `'a` is a bound on the lifetime of the underlying buffer (which means it is a bound
/// that buffer's underlying data. /// on the data in that buffer by transitivity).
#[derive(Debug)] #[derive(Debug)]
pub struct BorrowedCursor<'buf, 'data> { pub struct BorrowedCursor<'a> {
/// The underlying buffer. /// The underlying buffer.
buf: &'buf mut BorrowedBuf<'data>, // Safety invariant: we treat the type of buf as covariant in the lifetime of `BorrowedBuf` when
// we create a `BorrowedCursor`. This is only safe if we never replace `buf` by assigning into
// it, so don't do that!
buf: &'a mut BorrowedBuf<'a>,
/// The length of the filled portion of the underlying buffer at the time of the cursor's /// The length of the filled portion of the underlying buffer at the time of the cursor's
/// creation. /// creation.
start: usize, start: usize,
} }
impl<'buf, 'data> BorrowedCursor<'buf, 'data> { impl<'a> BorrowedCursor<'a> {
/// Clone this cursor. /// Reborrow this cursor by cloning it with a smaller lifetime.
/// ///
/// Since a cursor maintains unique access to its underlying buffer, the cloned cursor is not /// Since a cursor maintains unique access to its underlying buffer, the borrowed cursor is
/// accessible while the clone is alive. /// not accessible while the new cursor exists.
#[inline] #[inline]
pub fn clone<'this>(&'this mut self) -> BorrowedCursor<'this, 'data> { pub fn reborrow<'this>(&'this mut self) -> BorrowedCursor<'this> {
BorrowedCursor { buf: self.buf, start: self.start } BorrowedCursor {
// SAFETY: we never assign into `BorrowedCursor::buf`, so treating its
// lifetime covariantly is safe.
buf: unsafe {
mem::transmute::<&'this mut BorrowedBuf<'a>, &'this mut BorrowedBuf<'this>>(
self.buf,
)
},
start: self.start,
}
} }
/// Returns the available space in the cursor. /// Returns the available space in the cursor.
@ -170,8 +189,8 @@ impl<'buf, 'data> BorrowedCursor<'buf, 'data> {
/// Returns the number of bytes written to this cursor since it was created from a `BorrowedBuf`. /// Returns the number of bytes written to this cursor since it was created from a `BorrowedBuf`.
/// ///
/// Note that if this cursor is a clone of another, then the count returned is the count written /// Note that if this cursor is a reborrowed clone of another, then the count returned is the
/// via either cursor, not the count since the cursor was cloned. /// count written via either cursor, not the count since the cursor was reborrowed.
#[inline] #[inline]
pub fn written(&self) -> usize { pub fn written(&self) -> usize {
self.buf.filled - self.start self.buf.filled - self.start
@ -180,14 +199,14 @@ impl<'buf, 'data> BorrowedCursor<'buf, 'data> {
/// Returns a shared reference to the initialized portion of the cursor. /// Returns a shared reference to the initialized portion of the cursor.
#[inline] #[inline]
pub fn init_ref(&self) -> &[u8] { pub fn init_ref(&self) -> &[u8] {
//SAFETY: We only slice the initialized part of the buffer, which is always valid // SAFETY: We only slice the initialized part of the buffer, which is always valid
unsafe { MaybeUninit::slice_assume_init_ref(&self.buf.buf[self.buf.filled..self.buf.init]) } unsafe { MaybeUninit::slice_assume_init_ref(&self.buf.buf[self.buf.filled..self.buf.init]) }
} }
/// Returns a mutable reference to the initialized portion of the cursor. /// Returns a mutable reference to the initialized portion of the cursor.
#[inline] #[inline]
pub fn init_mut(&mut self) -> &mut [u8] { pub fn init_mut(&mut self) -> &mut [u8] {
//SAFETY: We only slice the initialized part of the buffer, which is always valid // SAFETY: We only slice the initialized part of the buffer, which is always valid
unsafe { unsafe {
MaybeUninit::slice_assume_init_mut(&mut self.buf.buf[self.buf.filled..self.buf.init]) MaybeUninit::slice_assume_init_mut(&mut self.buf.buf[self.buf.filled..self.buf.init])
} }
@ -275,7 +294,7 @@ impl<'buf, 'data> BorrowedCursor<'buf, 'data> {
} }
} }
impl<'buf, 'data> Write for BorrowedCursor<'buf, 'data> { impl<'a> Write for BorrowedCursor<'a> {
fn write(&mut self, buf: &[u8]) -> Result<usize> { fn write(&mut self, buf: &[u8]) -> Result<usize> {
self.append(buf); self.append(buf);
Ok(buf.len()) Ok(buf.len())

View File

@ -117,14 +117,14 @@ fn append() {
} }
#[test] #[test]
fn clone_written() { fn reborrow_written() {
let buf: &mut [_] = &mut [MaybeUninit::new(0); 32]; let buf: &mut [_] = &mut [MaybeUninit::new(0); 32];
let mut buf: BorrowedBuf<'_> = buf.into(); let mut buf: BorrowedBuf<'_> = buf.into();
let mut cursor = buf.unfilled(); let mut cursor = buf.unfilled();
cursor.append(&[1; 16]); cursor.append(&[1; 16]);
let mut cursor2 = cursor.clone(); let mut cursor2 = cursor.reborrow();
cursor2.append(&[2; 16]); cursor2.append(&[2; 16]);
assert_eq!(cursor2.written(), 32); assert_eq!(cursor2.written(), 32);

View File

@ -47,7 +47,7 @@ impl Read for Empty {
} }
#[inline] #[inline]
fn read_buf(&mut self, _cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { fn read_buf(&mut self, _cursor: BorrowedCursor<'_>) -> io::Result<()> {
Ok(()) Ok(())
} }
} }
@ -130,7 +130,7 @@ impl Read for Repeat {
Ok(buf.len()) Ok(buf.len())
} }
fn read_buf(&mut self, mut buf: BorrowedCursor<'_, '_>) -> io::Result<()> { fn read_buf(&mut self, mut buf: BorrowedCursor<'_>) -> io::Result<()> {
// SAFETY: No uninit bytes are being written // SAFETY: No uninit bytes are being written
for slot in unsafe { buf.as_mut() } { for slot in unsafe { buf.as_mut() } {
slot.write(self.byte); slot.write(self.byte);

View File

@ -312,7 +312,7 @@ impl File {
false false
} }
pub fn read_buf(&self, cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { pub fn read_buf(&self, cursor: BorrowedCursor<'_>) -> io::Result<()> {
crate::io::default_read_buf(|buf| self.read(buf), cursor) crate::io::default_read_buf(|buf| self.read(buf), cursor)
} }

View File

@ -358,7 +358,7 @@ impl File {
} }
} }
pub fn read_buf(&self, mut cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { pub fn read_buf(&self, mut cursor: BorrowedCursor<'_>) -> io::Result<()> {
unsafe { unsafe {
let len = cursor.capacity(); let len = cursor.capacity();
let mut out_num_bytes = MaybeUninit::uninit(); let mut out_num_bytes = MaybeUninit::uninit();

View File

@ -131,7 +131,7 @@ impl FileDesc {
} }
} }
pub fn read_buf(&self, mut cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { pub fn read_buf(&self, mut cursor: BorrowedCursor<'_>) -> io::Result<()> {
let ret = cvt(unsafe { let ret = cvt(unsafe {
libc::read( libc::read(
self.as_raw_fd(), self.as_raw_fd(),

View File

@ -1031,7 +1031,7 @@ impl File {
self.0.read_at(buf, offset) self.0.read_at(buf, offset)
} }
pub fn read_buf(&self, cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { pub fn read_buf(&self, cursor: BorrowedCursor<'_>) -> io::Result<()> {
self.0.read_buf(cursor) self.0.read_buf(cursor)
} }

View File

@ -214,7 +214,7 @@ impl File {
self.0 self.0
} }
pub fn read_buf(&self, _cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { pub fn read_buf(&self, _cursor: BorrowedCursor<'_>) -> io::Result<()> {
self.0 self.0
} }

View File

@ -439,7 +439,7 @@ impl File {
true true
} }
pub fn read_buf(&self, cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { pub fn read_buf(&self, cursor: BorrowedCursor<'_>) -> io::Result<()> {
crate::io::default_read_buf(|buf| self.read(buf), cursor) crate::io::default_read_buf(|buf| self.read(buf), cursor)
} }

View File

@ -415,7 +415,7 @@ impl File {
self.handle.read_at(buf, offset) self.handle.read_at(buf, offset)
} }
pub fn read_buf(&self, cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { pub fn read_buf(&self, cursor: BorrowedCursor<'_>) -> io::Result<()> {
self.handle.read_buf(cursor) self.handle.read_buf(cursor)
} }

View File

@ -112,7 +112,7 @@ impl Handle {
} }
} }
pub fn read_buf(&self, mut cursor: BorrowedCursor<'_, '_>) -> io::Result<()> { pub fn read_buf(&self, mut cursor: BorrowedCursor<'_>) -> io::Result<()> {
let res = let res =
unsafe { self.synchronous_read(cursor.as_mut().as_mut_ptr(), cursor.capacity(), None) }; unsafe { self.synchronous_read(cursor.as_mut().as_mut_ptr(), cursor.capacity(), None) };