clarify BufRead::{fill_buf, consume} docs #136177

hkBst · 2025-01-28T08:39:54Z

rustbot · 2025-01-28T08:40:03Z

rustbot has assigned @ibraheemdev.
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

Fixes rust-lang#85394

ibraheemdev

I like the primary change here, but unsure about some of the other changes.

ibraheemdev · 2025-02-11T03:18:07Z

library/std/src/io/mod.rs

@@ -2250,24 +2250,18 @@ fn skip_until<R: BufRead + ?Sized>(r: &mut R, delim: u8) -> Result<usize> {
 /// ```
 #[stable(feature = "rust1", since = "1.0.0")]
 pub trait BufRead: Read {
-    /// Returns the contents of the internal buffer, filling it with more data
-    /// from the inner reader if it is empty.
+    /// Returns the contents of the internal buffer, filling it with more data, via Read methods, if empty.


I'm not sure this is better. I would prefer the "inner reader" wording.

I found "inner reader" confusing, since it sounds like a member of a struct, while it is actually a supertrait of this trait.

ibraheemdev · 2025-02-11T03:21:43Z

library/std/src/io/mod.rs

-    /// This function will return an I/O error if the underlying reader was
-    /// read, but returned an error.
+    /// Passes on I/O errors from Read.


Similarly not sure if this is an improvement.

Same reasoning as for "inner reader", and more direct, though perhaps something like "its supertrait Read" would make this better?

ibraheemdev · 2025-02-11T03:22:26Z

library/std/src/io/mod.rs

-    /// Tells this buffer that `amt` bytes have been consumed from the buffer,
-    /// so they should no longer be returned in calls to `read`.
+    /// Marks the given `amount` of additional bytes from the internal buffer as having been read.
+    /// Subsequent calls to `read` return bytes that have not yet been so marked.


Suggested change

/// Subsequent calls to `read` return bytes that have not yet been so marked.

/// Subsequent calls to `read` will not return bytes that have been marked as consumed.

I'd really like to avoid phrasing this as a negative, and instead say what read will do as opposed to what it won't. Perhaps this would be better if "so marked" => "marked as read"?

ibraheemdev · 2025-02-11T03:23:51Z

library/std/src/io/mod.rs

    ///
    /// [`consume`]: BufRead::consume
    ///
-    /// An empty buffer returned indicates that the stream has reached EOF.
+    /// Returns an empty buffer to indicate that the stream has reached EOF.


This is a specific return condition, the new wording makes it sound like it always returns an empty buffer.

I see your point. Perhaps "to indicate that" => "when"?

ibraheemdev · 2025-02-11T03:25:04Z

library/std/src/io/mod.rs

+    /// This is a lower-level method and is meant to be used together with [`fill_buf`],
+    /// which can be used to fill the internal buffer via Read methods.


Can we keep some of the extra context from the original?

Suggested change

/// This is a lower-level method and is meant to be used together with [`fill_buf`],

/// which can be used to fill the internal buffer via Read methods.

/// This is a lower-level method and is meant to be used together with [`fill_buf`],

/// which can be used to fill the internal buffer via `Read` methods. This function does

/// not perform any I/O, it simply informs this object that some amount of

/// its buffer, returned from [`fill_buf`], has been consumed and should

/// no longer be returned.

ibraheemdev · 2025-02-11T03:26:17Z

library/std/src/io/mod.rs


-    /// Checks if the underlying `Read` has any data left to be read.
+    /// Checks if there is any data left to be `read`.


I also prefer the "underlying reader" wording here.

ibraheemdev · 2025-02-11T03:26:42Z

library/std/src/io/mod.rs

@@ -2324,6 +2313,10 @@ pub trait BufRead: Read {
    /// returned slice is empty (which means that there is no data left,
    /// since EOF is reached).
    ///
+    /// # Errors
+    ///
+    /// Passes on I/O errors from Read.


Same here.

Suggested change

/// Passes on I/O errors from Read.

/// This function will return an I/O error if the underlying reader was

/// read, but returned an error.

hkBst · 2025-03-02T19:30:55Z

@rustbot ready

rustbot assigned ibraheemdev Jan 28, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 28, 2025

This comment has been minimized.

Sign in to view

clarify BufRead::{fill_buf, consume} docs

51c5700

Fixes rust-lang#85394

hkBst force-pushed the patch-24 branch from 7409e41 to 51c5700 Compare January 28, 2025 09:36

ibraheemdev requested changes Feb 11, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clarify BufRead::{fill_buf, consume} docs #136177

clarify BufRead::{fill_buf, consume} docs #136177

hkBst commented Jan 28, 2025

rustbot commented Jan 28, 2025

This comment has been minimized.

ibraheemdev left a comment

ibraheemdev Feb 11, 2025

hkBst Feb 19, 2025 •

edited

Loading

ibraheemdev Feb 11, 2025

hkBst Feb 19, 2025 •

edited

Loading

ibraheemdev Feb 11, 2025

hkBst Feb 19, 2025

ibraheemdev Feb 11, 2025

hkBst Feb 19, 2025

ibraheemdev Feb 11, 2025

ibraheemdev Feb 11, 2025

ibraheemdev Feb 11, 2025

hkBst commented Mar 2, 2025

	/// Subsequent calls to `read` return bytes that have not yet been so marked.
	/// Subsequent calls to `read` will not return bytes that have been marked as consumed.

		/// This is a lower-level method and is meant to be used together with [`fill_buf`],
		/// which can be used to fill the internal buffer via Read methods.

-    /// This is a lower-level method and is meant to be used together with [`fill_buf`],
-    /// which can be used to fill the internal buffer via Read methods.
+    /// This is a lower-level method and is meant to be used together with [`fill_buf`],
+    /// which can be used to fill the internal buffer via `Read` methods. This function does
+    /// not perform any I/O, it simply informs this object that some amount of
+    /// its buffer, returned from [`fill_buf`], has been consumed and should
+    /// no longer be returned.


		/// Checks if the underlying `Read` has any data left to be read.
		/// Checks if there is any data left to be `read`.

	/// Passes on I/O errors from Read.
	/// This function will return an I/O error if the underlying reader was
	/// read, but returned an error.

clarify BufRead::{fill_buf, consume} docs #136177

Are you sure you want to change the base?

clarify BufRead::{fill_buf, consume} docs #136177

Conversation

hkBst commented Jan 28, 2025

rustbot commented Jan 28, 2025

This comment has been minimized.

ibraheemdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkBst Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkBst Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkBst commented Mar 2, 2025

hkBst Feb 19, 2025 •

edited

Loading

hkBst Feb 19, 2025 •

edited

Loading