Skip to content

Commit

Permalink
Address reviewer comments
Browse files Browse the repository at this point in the history
Signed-off-by: Nick Cameron <nrc@ncameron.org>
  • Loading branch information
nrc committed Aug 18, 2022
1 parent 1a2122f commit ac70aea
Show file tree
Hide file tree
Showing 18 changed files with 74 additions and 55 deletions.
4 changes: 2 additions & 2 deletions library/std/src/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,7 @@ impl Read for File {
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)
}

Expand Down Expand Up @@ -755,7 +755,7 @@ impl Read for &File {
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)
}

Expand Down
4 changes: 2 additions & 2 deletions library/std/src/io/buffered/bufreader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ impl<R: Read> Read for BufReader<R> {
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
// (larger than our internal buffer), bypass our internal buffer
// entirely.
Expand All @@ -278,7 +278,7 @@ impl<R: Read> Read for BufReader<R> {
let prev = cursor.written();

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

Expand Down
2 changes: 1 addition & 1 deletion library/std/src/io/buffered/bufreader/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ impl Buffer {
if 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.
unsafe {
buf.set_init(self.filled);
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/io/copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ impl<I: Write> BufferedCopySpec for BufWriter<I> {

if read_buf.capacity() >= DEFAULT_BUF_SIZE {
let mut cursor = read_buf.unfilled();
match reader.read_buf(cursor.clone()) {
match reader.read_buf(cursor.reborrow()) {
Ok(()) => {
let bytes_read = cursor.written();

Expand Down
4 changes: 2 additions & 2 deletions library/std/src/io/cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,10 +323,10 @@ where
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();

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;

Expand Down
8 changes: 4 additions & 4 deletions library/std/src/io/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ impl<R: Read + ?Sized> Read for &mut R {
}

#[inline]
fn read_buf(&mut self, cursor: BorrowedCursor<'_, '_>) -> io::Result<()> {
fn read_buf(&mut self, cursor: BorrowedCursor<'_>) -> io::Result<()> {
(**self).read_buf(cursor)
}

Expand Down Expand Up @@ -125,7 +125,7 @@ impl<R: Read + ?Sized> Read for Box<R> {
}

#[inline]
fn read_buf(&mut self, cursor: BorrowedCursor<'_, '_>) -> io::Result<()> {
fn read_buf(&mut self, cursor: BorrowedCursor<'_>) -> io::Result<()> {
(**self).read_buf(cursor)
}

Expand Down Expand Up @@ -249,7 +249,7 @@ impl Read for &[u8] {
}

#[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 (a, b) = self.split_at(amt);

Expand Down Expand Up @@ -427,7 +427,7 @@ impl<A: Allocator> Read for VecDeque<u8, A> {
}

#[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 n = cmp::min(cursor.capacity(), front.len());
Read::read_buf(front, cursor)?;
Expand Down
16 changes: 8 additions & 8 deletions library/std/src/io/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
match r.read_buf(cursor.clone()) {
match r.read_buf(cursor.reborrow()) {
Ok(()) => {}
Err(e) if e.kind() == ErrorKind::Interrupted => continue,
Err(e) => return Err(e),
Expand Down Expand Up @@ -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
F: FnOnce(&mut [u8]) -> Result<usize>,
{
Expand Down Expand Up @@ -812,7 +812,7 @@ pub trait Read {
///
/// The default implementation delegates to `read`.
#[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)
}

Expand All @@ -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
/// allow use with uninitialized buffers.
#[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 {
let prev_written = cursor.written();
match self.read_buf(cursor.clone()) {
match self.read_buf(cursor.reborrow()) {
Ok(()) => {}
Err(e) if e.kind() == ErrorKind::Interrupted => continue,
Err(e) => return Err(e),
Expand Down Expand Up @@ -2586,7 +2586,7 @@ impl<T: Read> Read for Take<T> {
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
if self.limit == 0 {
return Ok(());
Expand All @@ -2609,7 +2609,7 @@ impl<T: Read> Read for Take<T> {
}

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 filled = sliced_buf.len();
Expand All @@ -2626,7 +2626,7 @@ impl<T: Read> Read for Take<T> {
self.limit -= filled as u64;
} else {
let written = buf.written();
self.inner.read_buf(buf.clone())?;
self.inner.read_buf(buf.reborrow())?;
self.limit -= (buf.written() - written) as u64;
}

Expand Down
65 changes: 42 additions & 23 deletions library/std/src/io/readbuf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ mod tests;
use crate::cmp;
use crate::fmt::{self, Debug, Formatter};
use crate::io::{Result, Write};
use crate::mem::MaybeUninit;
use crate::mem::{self, MaybeUninit};

/// A borrowed byte buffer which is incrementally filled and initialized.
///
Expand All @@ -23,9 +23,9 @@ use crate::mem::MaybeUninit;
/// ```
///
/// 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
/// is read-only. 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
/// (`&mut`). The `BorrowedBuf` can be configured (e.g., using `clear` or `set_init`), but cannot be
/// 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 as a
/// write-only iterator).
///
/// The lifetime `'data` is a bound on the lifetime of the underlying data.
Expand Down Expand Up @@ -55,7 +55,7 @@ impl<'data> From<&'data mut [u8]> for BorrowedBuf<'data> {
let len = slice.len();

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() },
filled: 0,
init: len,
Expand Down Expand Up @@ -95,14 +95,21 @@ impl<'data> BorrowedBuf<'data> {
/// Returns a shared reference to the filled portion of the buffer.
#[inline]
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]) }
}

/// Returns a cursor over the unfilled part of the buffer.
#[inline]
pub fn unfilled<'this>(&'this mut self) -> BorrowedCursor<'this, 'data> {
BorrowedCursor { start: self.filled, buf: self }
pub fn unfilled<'this>(&'this mut self) -> BorrowedCursor<'this> {
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.
Expand Down Expand Up @@ -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
/// 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
/// that buffer's underlying data.
/// The lifetime `'a` is a bound on the lifetime of the underlying buffer (which means it is a bound
/// on the data in that buffer by transitivity).
#[derive(Debug)]
pub struct BorrowedCursor<'buf, 'data> {
pub struct BorrowedCursor<'a> {
/// 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
/// creation.
start: usize,
}

impl<'buf, 'data> BorrowedCursor<'buf, 'data> {
/// Clone this cursor.
impl<'a> BorrowedCursor<'a> {
/// 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
/// accessible while the clone is alive.
/// Since a cursor maintains unique access to its underlying buffer, the borrowed cursor is
/// not accessible while the new cursor exists.
#[inline]
pub fn clone<'this>(&'this mut self) -> BorrowedCursor<'this, 'data> {
BorrowedCursor { buf: self.buf, start: self.start }
pub fn reborrow<'this>(&'this mut self) -> BorrowedCursor<'this> {
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.
Expand All @@ -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`.
///
/// Note that if this cursor is a clone of another, then the count returned is the count written
/// via either cursor, not the count since the cursor was cloned.
/// Note that if this cursor is a reborrowed clone of another, then the count returned is the
/// count written via either cursor, not the count since the cursor was reborrowed.
#[inline]
pub fn written(&self) -> usize {
self.buf.filled - self.start
Expand All @@ -180,14 +199,14 @@ impl<'buf, 'data> BorrowedCursor<'buf, 'data> {
/// Returns a shared reference to the initialized portion of the cursor.
#[inline]
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]) }
}

/// Returns a mutable reference to the initialized portion of the cursor.
#[inline]
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 {
MaybeUninit::slice_assume_init_mut(&mut self.buf.buf[self.buf.filled..self.buf.init])
}
Expand Down Expand Up @@ -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> {
self.append(buf);
Ok(buf.len())
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/io/readbuf/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,14 @@ fn append() {
}

#[test]
fn clone_written() {
fn reborrow_written() {
let buf: &mut [_] = &mut [MaybeUninit::new(0); 32];
let mut buf: BorrowedBuf<'_> = buf.into();

let mut cursor = buf.unfilled();
cursor.append(&[1; 16]);

let mut cursor2 = cursor.clone();
let mut cursor2 = cursor.reborrow();
cursor2.append(&[2; 16]);

assert_eq!(cursor2.written(), 32);
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/io/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ impl Read for Empty {
}

#[inline]
fn read_buf(&mut self, _cursor: BorrowedCursor<'_, '_>) -> io::Result<()> {
fn read_buf(&mut self, _cursor: BorrowedCursor<'_>) -> io::Result<()> {
Ok(())
}
}
Expand Down Expand Up @@ -130,7 +130,7 @@ impl Read for Repeat {
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
for slot in unsafe { buf.as_mut() } {
slot.write(self.byte);
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/hermit/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ impl File {
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)
}

Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/solid/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
let len = cursor.capacity();
let mut out_num_bytes = MaybeUninit::uninit();
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/unix/fd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
libc::read(
self.as_raw_fd(),
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/unix/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1031,7 +1031,7 @@ impl File {
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)
}

Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/unsupported/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ impl File {
self.0
}

pub fn read_buf(&self, _cursor: BorrowedCursor<'_, '_>) -> io::Result<()> {
pub fn read_buf(&self, _cursor: BorrowedCursor<'_>) -> io::Result<()> {
self.0
}

Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/wasi/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ impl File {
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)
}

Expand Down
Loading

0 comments on commit ac70aea

Please sign in to comment.