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

io: Add the Peek trait #87620

Closed
wants to merge 1 commit into from
Closed

io: Add the Peek trait #87620

wants to merge 1 commit into from

Conversation

LinkTed
Copy link
Contributor

@LinkTed LinkTed commented Jul 30, 2021

Add the trait for each struct which has a peek function.

Motivation

This new Trait make it possible to write a generic, where a stream is required, in which the data is not removed from the queue. Unfortunately, there are already peek method for some struct for example TcpStream, which make it not consistent (dirty).

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2021
@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 31, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@bors
Copy link
Contributor

bors commented Aug 20, 2021

☔ The latest upstream changes (presumably #87329) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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 Sep 6, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@LinkTed Can you please address the merge conflicts?
please set S-waiitng-on-review afterwards

@LinkTed
Copy link
Contributor Author

LinkTed commented Sep 6, 2021

Ping from triage:
@LinkTed Can you please address the merge conflicts?
please set S-waiitng-on-review afterwards

Okay, I will that.

@LinkTed
Copy link
Contributor Author

LinkTed commented Sep 10, 2021

@JohnCSimon

I rebased it, but it seems that I cannot change the label to S-waiitng-on-review.

@dylni
Copy link
Contributor

dylni commented Sep 26, 2021

@rustbot label: -S-waiting-on-author +S-waiting-on-review

@rustbot 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 Sep 26, 2021
@kennytm
Copy link
Member

kennytm commented Sep 27, 2021

  1. should this be implemented for UdpSocket?
  2. can https://docs.rs/std-io-peek/0.1.0/std_io_peek/ be used instead? (This PR additionally implements for UnixStream and UnixDatagram but both are unstable under Tracking Issue for feature(unix_socket_peek) #76923; adding an unstable Peek trait isn't going to change that)

@LinkTed
Copy link
Contributor Author

LinkTed commented Oct 2, 2021

1. should this be implemented for [`UdpSocket`](https://doc.rust-lang.org/std/net/struct.UdpSocket.html#method.peek)?

I will add implement for UdpSocket too.

2. can https://docs.rs/std-io-peek/0.1.0/std_io_peek/ be used instead? (This PR additionally implements for UnixStream and UnixDatagram but both are unstable under [Tracking Issue for feature(unix_socket_peek) #76923](https://github.com/rust-lang/rust/issues/76923); adding an unstable Peek trait isn't going to change that)

Should I create a new tracking issue?

@rust-log-analyzer

This comment has been minimized.

Add the trait for each struct which has a peek function.
@kennytm
Copy link
Member

kennytm commented Oct 2, 2021

@LinkTed

Should I create a new tracking issue?

What I mean is, currently stable code can't use Unix{Stream,Datagram}::peek as they are unstable. But stable code can't use this Peek trait either since it is also unstable.

If Unix{Stream,Datagram}::peek is stabilized then an external crate like std-io-peek can also do the same job. Besides, std itself does not rely on the Peek trait anywhere.

So the actual question of this is, whether the Peek trait is really needed to be frozen into std? This is where I have doubt.

@LinkTed
Copy link
Contributor Author

LinkTed commented Oct 4, 2021

@LinkTed

Should I create a new tracking issue?

What I mean is, currently stable code can't use Unix{Stream,Datagram}::peek as they are unstable. But stable code can't use this Peek trait either since it is also unstable.

I understand that, so this PR has to be merged after the stabilization.

If Unix{Stream,Datagram}::peek is stabilized then an external crate like std-io-peek can also do the same job. Besides, std itself does not rely on the Peek trait anywhere.

The reason, why it does not rely on anywhere, is that it was not there yet.

So the actual question of this is, whether the Peek trait is really needed to be frozen into std? This is where I have doubt.

With this argument, then many things should not be in std. I don't see any harm, that it would cause.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 24, 2021
@JohnCSimon JohnCSimon added the I-needs-decision Issue: In need of a decision. label Nov 16, 2021
@JohnCSimon JohnCSimon removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2021
@JohnCSimon JohnCSimon added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2021
@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/libs-api meeting. We came to consensus on two points:

  • As a new trait, this should use ReadBuf, rather than using a &mut [u8] buffer that requires initialization.
  • We'd like to have some details about the specific use cases for having a trait abstracting this.

@joshtriplett joshtriplett removed the I-needs-decision Issue: In need of a decision. label Dec 8, 2021
@bors
Copy link
Contributor

bors commented Dec 9, 2021

☔ The latest upstream changes (presumably #81156) made this pull request unmergeable. Please resolve the merge conflicts.

@LinkTed
Copy link
Contributor Author

LinkTed commented Dec 12, 2021

We discussed this in today's @rust-lang/libs-api meeting. We came to consensus on two points:

* As a new trait, this should use `ReadBuf`, rather than using a `&mut [u8]` buffer that requires initialization.

* We'd like to have some details about the specific use cases for having a trait abstracting this.

My use case is:
The user can write functions, where it takes the bytes from the stream (TcpStream, UnixStream, TlsStream) until a specific character, for example a new line, without using BufReader. To do so, he has to check if the character, he is looking for, is in the current buffer of the stream, by using peek. And then, read only until the character.

@mbartlett21
Copy link
Contributor

This seems to be BufRead, except that the caller provides the buffer, not the type implementing the trait. Is that right?

@LinkTed
Copy link
Contributor Author

LinkTed commented Dec 18, 2021

No, really. It is like BufRead, where after a read line the buffer is empty.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2022
@JohnCSimon
Copy link
Member

Triage:
@kennytm can you please review this?

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2022
@Mark-Simulacrum
Copy link
Member

Based on #87620 (comment), it sounds like this will need an evaluation of the use case presented in #87620 (comment) to determine if there's a desire to add the API.

Marking as waiting-on-team for that.

These days, it's probably the case that the discussion should be had on an ACP, but I'm not sure what our policy on pre-existing PRs is there.

@yaahc
Copy link
Member

yaahc commented Aug 1, 2022

I'm not sure what our policy on pre-existing PRs is there.

I think we've been tending to ask existing PRs to go through the ACP process.

@Mark-Simulacrum Mark-Simulacrum 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Aug 1, 2022
@JohnCSimon
Copy link
Member

Triage:
@yaahc - What (if anything) should the author do to move forward with this PR?

cc: @LinkTed

@Dylan-DPC
Copy link
Member

Closing this as it is inactive and would need an ACP

@Dylan-DPC Dylan-DPC closed this Oct 27, 2022
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.