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

RFC: Add size hint methods on Reader and Writer #46

Closed

Conversation

SimonSapin
Copy link
Contributor

No description provided.

@alexcrichton
Copy link
Member

One thing I'd be a little worried about is how wordy writer wrappers will become. When a type wraps an inner writer, it already has to remember to override flush (which is unfortunate, and perhaps should be fixed), and this would be adding another method which needs to be remembered to get overridden. That's not necessarily a reason to block this, but it's something to keep in mind.

I personally think that this belongs as a separate trait because most writers aren't pre-allocating space. Essentially MemWriter is the only one that would have an implementation of this method (currently).

@thestinger
Copy link

Pre-allocating space is also important when writing to the disk. If fallocate is not called and a bunch of data is appended, there's going to be fragmentation.

@SimonSapin
Copy link
Contributor Author

When a type wraps an inner writer, it already has to remember to override flush

If a writer wrapper forgets to override reserve_additional, it misses some optimization opportunity but it is still correct. reserve_additional doing nothing (the default) is a perfectly valid behavior for any writer.

I personally think that this belongs as a separate trait

This doesn’t really work for generic code that wants to accept any writer. The point is to provide this information to the writer, and leave it to the implementation to decide what to do with it (if anything). If this is a separate trait, a writer that doesn’t do anything with this information would still need to implement the trait in order to be used with code that provides this information.

@SimonSapin
Copy link
Contributor Author

I suppose a separate trait would work if we have "specialization" (rust-lang/rust#7059).

* Add size_hint on Reader, like on Iterator
* Highlight the precedent of Iterator::size_hint
* Rename reserve_additional to reserve
* Use an estimated range (lower bound and optional upper bound)
  like size_hint, rather than a single estimated number.
@SimonSapin
Copy link
Contributor Author

I just pushed an update. The text is almost completely rewritten, but the spirit is the same. The main changes are:

  • Add size_hint on Reader, like on Iterator
  • Highlight the precedent of Iterator::size_hint
  • Rename reserve_additional to reserve
  • Use an estimated range (lower bound and optional upper bound) like size_hint, rather than a single estimated number.

@SimonSapin SimonSapin changed the title RFC: Add a size hint method to allow Writer objects to pre-allocate space RFC: Add size hint methods on Reader and Writer Apr 29, 2014
@brson
Copy link
Contributor

brson commented Jun 11, 2014

Discussed at https://github.com/mozilla/rust/wiki/Meeting-weekly-2014-06-10, the feeling is that none of us have seen precedent for these operations in other I/O libraries and we don't want to risk breaking the mold here. Not accepted. Sorry.

@brson brson closed this Jun 11, 2014
@SimonSapin
Copy link
Contributor Author

Unfortunately it seems that no one was "championing" this RFC when it was discussed. I accept the team’s decision, but just for my curiosity: was there a precedent for the size_hint method on iterators? How is Reader::size_hint more risky than Iterator::size_hint?

@brson
Copy link
Contributor

brson commented Jun 19, 2014

@SimonSapin I don't know if there is precedent for iterator size hints. @thestinger might know.

@thestinger
Copy link

Python has a size hint for iterators, and Rust's size hint was inspired by that feature. I altered it to provide both a lower bound and optional upper bound, because Python's flexible definition can be problematic:

http://legacy.python.org/dev/peps/pep-0424/

@erickt erickt mentioned this pull request Jan 14, 2015
withoutboats pushed a commit to withoutboats/rfcs that referenced this pull request Jan 15, 2017
wycats added a commit to wycats/rust-rfcs that referenced this pull request Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants