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

Add fn skip(n: uint) with a default implementation to Reader #13989

Closed
carllerche opened this issue May 6, 2014 · 12 comments
Closed

Add fn skip(n: uint) with a default implementation to Reader #13989

carllerche opened this issue May 6, 2014 · 12 comments
Labels
P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@carllerche
Copy link
Member

Reader should have an extra fn that allows skipping a number of bytes. The default implementation could look something like:

fn skip(n: uint) { let _ = self.read(Vec::from_elem(n, 0u8); }

However, BufReader could implement the fn by simply updating the internal cursor. This would allow more efficient skipping w/o having to require Seek.

@pongad
Copy link
Contributor

pongad commented May 14, 2014

This seems useful though reader does have a few methods on it already. I'm wondering what @alexcrichton thinks?
Edit: I'm not sure if this is a worthwhile change anymore. We can easily to reader.read_exact(n) to achieve the same thing.

@alexcrichton
Copy link
Member

I'm worried about adding more methods to readers that downstream consumers need to consider to override. Having one flush() method is unfortunate, and I think time should be taken to consider whether this is justified to add it or not.

I don't think that we can quite use read_exact for this because in theory skip doesn't actually have to read any data. It does sound quite similar to seek though...

@nox
Copy link
Contributor

nox commented Jul 11, 2014

Any news on this? I'm writing an IFF reader and I want users to be able to skip chunk bodies, requiring Seek for just this seems overkill.

@wycats
Copy link
Contributor

wycats commented Jul 20, 2014

The problem is that given today's (and likely future) coherence rules, you cannot create a trait that uses Seek if available but otherwise falls back to a less efficient Reader:

impl<T: Seek> PossiblyInefficientSeek for T {
    // ...
}

impl<T: Reader> PossiblyInefficientSeek for T {
    // ...
}

This is actually a decent motivating use-case for allowing an implementation of a trait to refine an existing implementation defined on a superset of types.

@wycats
Copy link
Contributor

wycats commented Jul 20, 2014

@pongad I do not think read_exact is appropriate, since it requires allocating a buffer of n size even if the backend supports efficient skipping.

@nikomatsakis
Copy link
Contributor

@alexcrichton it is true that having too many random convenience methods can be a problem, but I think that it makes sense to add methods for operations that can be implemented much more efficiently in some contexts than others. In other words, while skip is technically implementable as a read, there is a big efficiency difference between a call to fseek and reading tons of bytes.

@steveklabnik
Copy link
Member

@carllerche did this end up getting addressed in io reform?

@carllerche
Copy link
Member Author

@steveklabnik Not yet. I think (hope) it is coming pre 1.0 though. cc @aturon

@aturon
Copy link
Member

aturon commented Feb 16, 2015

Nominating for 1.0-beta P-high. (Should be trivial.)

@pnkfelix
Copy link
Member

P-high, I-needs-decision (not 1.0 beta blocker)

@pnkfelix pnkfelix added P-medium Medium priority I-needs-decision Issue: In need of a decision. and removed I-nominated labels Feb 19, 2015
@huonw huonw added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed I-needs-decision Issue: In need of a decision. labels Jan 8, 2016
@alexcrichton
Copy link
Member

In today's day and age this sort of addition (to a core trait like Read) will require an RFC, so I'm going to close this in favor of that course of action.

@youknowone
Copy link
Contributor

#53294

bors added a commit to rust-lang-ci/rust that referenced this issue Feb 13, 2023
internal: Remove hover fallback in favor of ranged hover

The fallback is usually more annoying than useful at this point (it messes with the range of diagnostic popups a lot), we now have a ranged hover to check the type of something which works a lot better.

Closes rust-lang/rust-analyzer#11602
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.