std::io::Take: Clarify & optimize BorrowedBuf::set_init usage.

[briansmith]

Don't initialize buf if it was already initialized. Clarify safety comments.

Move the buf.advance() call to make the initialization more like
calling buf.ensure_init(), then clarify how the code here is an
optimized variant of ensure_init.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 8 candidates

[Mark-Simulacrum]

Mostly questions at this point, not necessarily blockers for merging. Thanks!

View changes since this review

[Mark-Simulacrum]

Doesn't moving this up cause limit to be wrong when we decide what to initialize below? It would need to be limit - filled now, unless I'm missing something?

Either way I think a comment would be good noting whether order matters (or not), and if my suspicion of breakage is right is accurate then we probably should try to come up with a test...

[Mark-Simulacrum]

While trying to understand this code I edited locally, if the renaming makes sense to you I'd be happy to include it here:

diff --git a/library/std/src/io/mod.rs b/library/std/src/io/mod.rs
index 1166ba8baf4..babc3c1d836 100644
--- a/library/std/src/io/mod.rs
+++ b/library/std/src/io/mod.rs
@@ -3070,60 +3070,58 @@ fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
         Ok(n)
     }
 
-    fn read_buf(&mut self, mut buf: BorrowedCursor<'_>) -> Result<()> {
+    fn read_buf(&mut self, mut output_buffer: BorrowedCursor<'_>) -> Result<()> {
         // Don't call into inner reader at all at EOF because it may still block
         if self.limit == 0 {
             return Ok(());
         }
 
-        if self.limit < buf.capacity() as u64 {
+        if self.limit < output_buffer.capacity() as u64 {
             // The condition above guarantees that `self.limit` fits in `usize`.
             let limit = self.limit as usize;
 
-            let is_init = buf.is_init();
-
-            // SAFETY: no uninit data is written to ibuf
-            let ibuf = unsafe { &mut buf.as_mut()[..limit] };
+            let is_init = output_buffer.is_init();
 
-            let mut sliced_buf: BorrowedBuf<'_> = ibuf.into();
+            // SAFETY: no uninit data is written to limited_output
+            let limited_output = unsafe { &mut output_buffer.as_mut()[..limit] };
+            let mut limited_output: BorrowedBuf<'_> = limited_output.into();
 
             if is_init {
-                // SAFETY: `sliced_buf` is a subslice of `buf`, so if `buf` was initialized then
-                // `sliced_buf` is.
-                unsafe { sliced_buf.set_init() };
+                // SAFETY: `limited_output` is a subslice of `output_buffer`, so if `output_buffer` was initialized then
+                // `limited_output` is.
+                unsafe { limited_output.set_init() };
             }
 
-            let mut cursor = sliced_buf.unfilled();
-            let result = self.inner.read_buf(cursor.reborrow());
+            let result = self.inner.read_buf(limited_output.unfilled());
 
-            let should_init = cursor.is_init();
-            let filled = sliced_buf.len();
+            let did_initialize_to_limit = limited_output.is_init();
+            let filled = limited_output.len();
 
-            // cursor / sliced_buf / ibuf must drop here
+            // limited_output must drop here
 
             // Avoid accidentally quadratic behaviour by initializing the whole
             // cursor if only part of it was initialized.
-            if should_init {
+            if did_initialize_to_limit && !is_init {
                 // SAFETY: no uninit data is written
-                let uninit = unsafe { &mut buf.as_mut()[limit..] };
+                let uninit = unsafe { &mut output_buffer.as_mut()[limit..] };
                 uninit.write_filled(0);
                 // SAFETY: all bytes that were not initialized by `T::read_buf`
                 // have just been written to.
-                unsafe { buf.set_init() };
+                unsafe { output_buffer.set_init() };
             }
 
             unsafe {
                 // SAFETY: filled bytes have been filled
-                buf.advance(filled);
+                output_buffer.advance(filled);
             }
 
             self.limit -= filled as u64;
 
             result
         } else {
-            let written = buf.written();
-            let result = self.inner.read_buf(buf.reborrow());
-            self.limit -= (buf.written() - written) as u64;
+            let written = output_buffer.written();
+            let result = self.inner.read_buf(output_buffer.reborrow());
+            self.limit -= (output_buffer.written() - written) as u64;
             result
         }
     }

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.