Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BufReader/Writer extension methods: is_empty, buffer #45323

Open
fintelia opened this issue Oct 16, 2017 · 36 comments

Comments

Projects
None yet
9 participants
@fintelia
Copy link
Contributor

commented Oct 16, 2017

Update: this is the tracking issue for:

impl<R: Read> BufReader<R> {
    pub fn buffer(&self) -> &[u8] {…}

    #[rustc_deprecated(since = "1.26.0", reason = "use .buffer().is_empty() instead")]
    pub fn is_empty(&self) -> bool {…}
}

is_empty is both unstable and deprecated, it should be removed.


There is currently no way to tell whether there is any data buffered inside a BufReader. This is unfortunate because it means that an application has no way to know whether calls to read() and fill_buffer() will return instantly or trigger a potentially expensive call to the underlying Reader. I propose adding an is_empty() method to BufReader to fill this gap.

bors added a commit that referenced this issue Nov 6, 2017

Auto merge of #45369 - fintelia:patch-1, r=BurntSushi
Implement is_empty() for BufReader

Simple implementation of `is_empty` method for BufReader so it is possible to tell whether there is any data in its buffer.

I didn't know correct stability annotation to place on the function. Presumably there is no reason to place this feature behind a feature flag, but I wasn't sure how to tag it as an unstable feature without that.

CC: #45323
@jonhoo

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2017

@kennytm now that #45369 has been merged, I'm guessing this should get the C-tracking-issue label?

@dbrgn

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

What's left for this to stabilize?

I'm parsing bytes from a BufReader to MsgPack using read_value. The function will stop parsing once a valid MsgPack value has been found. But in my application the parsing should fail if the buffer hasn't been consumed completely. Checking buf.is_empty() would probably be the cleanest way to do so.

@jonhoo

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

@dbrgn the BufReader::buf_bytes method proposed in #48022 may also be useful for that purpose, though that may take even longer to land.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2018

This is the tracking issue for:

impl<R: Read> BufReader<R> {
    pub fn is_empty(&self) -> bool { /*…*/ }
}

Looks good to me to stabilize.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Mar 17, 2018

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 17, 2018

This seems fine to stabilize, but it seems like it'd be more useful to know how many bytes are buffered, rather than just that there are some number of bytes.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2018

Why not both? Slices have is_empty even though it’s somewhat redundant with len.

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 17, 2018

For sure. Would it be reasonable to try to squeeze that into this stabilization?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2018

Sounds ok to me, though we should probably (re)start FCP after the implementation is merged.

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 17, 2018

Naming wise, I don't think len fits since that seems to me like it'd refer to the length of the entire reader rather than just the current buffered data (similarly is_empty is a bit confusing). How about buffered_len and buffer_is_empty?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2018

Agreed. Sounds good to me.

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 17, 2018

Alternatively, we could add fn buffer(&self) -> &[u8] to just return the buffer, which is also a thing people want separately from this issue. At that point r.buffer().len() seems not much worse than r.buffer_len() so we could have one method instead of three.

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 18, 2018

#49139 adds buffer and deprecates is_empty.

@rfcbot

This comment has been minimized.

Copy link

commented Mar 19, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@fintelia

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2018

With @sfackler's alternative (and IMO better) proposal just introduced, should this still be going into its final comment period?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

@rfcbot fcp cancel

Let’s start again after buffer() has landed.

@rfcbot

This comment has been minimized.

Copy link

commented Mar 19, 2018

@SimonSapin proposal cancelled.

@jonhoo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

We should likely also add a .buffer() method to BufWriter for consistency?

@sfackler

This comment has been minimized.

Copy link
Member

commented May 30, 2018

That can't be done backwards compatibly.

@jonhoo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

@sfackler why not? It's just a struct, right? Just like BufReader.

@sfackler

This comment has been minimized.

Copy link
Member

commented May 30, 2018

Oh derp I somehow read that as BufWrite.

@jonhoo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

Haha, no worries. Is there even a BufWrite trait?

@sfackler

This comment has been minimized.

Copy link
Member

commented May 30, 2018

No there isn't >_>

Off of the topic of me needing more caffeine, it definitely seems reasonable to add buffer to BufWriter, and fold it into the same feature gate as BufReader's.

@Mark-Simulacrum Mark-Simulacrum changed the title BufReader should have an is_empty() method BufReader/Writer extension methods: is_empty, buffer Jun 2, 2018

@fintelia

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

It has been a while since buffer landed. Is this ready to stabilize?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

BufReader::buffer is implemented, but BufWriter::buffer discussed above is not. (I’ve updated the issue’s description with the current status.)

@fintelia

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

Wait, we aren't proposing to stabilize the (already deprecated) is_empty method are we?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

No, sorry I didn’t clarify. An API marked both unstable and deprecated is to be removed (after something like a couple release cycles of deprecation period, which we’re at in this case).

@zslayton

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

I'm excited to see that this is so close to landing. Is this still waiting on an implementation of BufWriter::buffer? Would it be possible to track that feature separately to unblock this one?

kennytm added a commit to kennytm/rust that referenced this issue Nov 14, 2018

pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 15, 2018

@fintelia

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

Bump. Is there anything else blocking stabilization?

@edef1c

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

Is there any particular reason not to add fn buffer(&self) -> &[u8] { &[] } to BufWrite?
It seems like a useful enough general-purpose API ("get me however many bytes you can easily give me"), and I found myself looking for such a method while working on parser tooling.
Individual implementations could make stricter guarantees about the lower bounds, of which "at least the unconsumed fraction of the last fill_buf" would be a common one.

@fintelia

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Yeah, this has been a while. @SimonSapin @sfackler is this ready for a vote to stabilize?

@fintelia

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

This issue seems to have been stuck in a strange limbo state for quite a while now. Pinging more people the rfcbot mentioned... @alexcrichton @aturon @dtolnay @withoutboats

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Sorry we dropped the ball here. This is now one method on the BufReader and BufWriter structs.

impl<R> BufReader<R> {
    pub fn buffer(&self) -> &[u8] {…}
}
impl<W: Write> BufWriter<W> {
    pub fn buffer(&self) -> &[u8] {…}
}

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented May 10, 2019

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

@edef1c There is nothing called BufWrite in std. Did you mean the BufRead trait? A downside is that to add it now we would have to make it a default method, which would (at first) have an inconsistent behavior with fill_buf for existing impls. We can update impls in std at the same time, but that leaves impls in other crates.

@rfcbot

This comment has been minimized.

Copy link

commented May 15, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.