-
Notifications
You must be signed in to change notification settings - Fork 1.7k
RFC for read_all #980
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 for read_all #980
Changes from 3 commits
288b1b5
6566a1d
1e457ad
c7f633b
d5284eb
f65d966
ddf9eff
585b289
4df6899
4732352
894ab34
7cd058d
67491eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| - Feature Name: read_all | ||
| - Start Date: 2015-03-15 | ||
| - RFC PR: (leave this empty) | ||
| - Rust Issue: (leave this empty) | ||
|
|
||
| # Summary | ||
|
|
||
| Rust's Write trait has write_all, which attempts to write an entire | ||
| buffer. This proposal adds read_all, which attempts to read a fixed | ||
| number of bytes into a given buffer. | ||
|
|
||
| # Motivation | ||
|
|
||
| The new read_all method will allow programs to read from disk without | ||
| having to write their own read loops. Most Rust programs which need | ||
| to read from disk will prefer this to the plain read function. Many C | ||
| programs have the same need, and solve it the same way (e.g. git has | ||
| read_in_full). Here's one example of a Rust library doing this: | ||
| https://github.com/BurntSushi/byteorder/blob/master/src/new.rs#L184 | ||
|
|
||
| # Detailed design | ||
|
|
||
| The read_all function will take a mutable, borrowed slice of u8 to | ||
| read into, and will attempt to fill that entire slice with data. | ||
|
|
||
| It will loop, calling read() once per iteration and attempting to read | ||
| the remaining amount of data. If read returns EINTR, the loop will | ||
| retry. If there are no more bytes to read (as signalled by a return | ||
| of Ok(0) from read()), a new error type, ErrorKind::ShortRead(usize), | ||
| will be returned. ShortRead includes the number of bytes successfully | ||
| read. In the event of another error, that error will be | ||
| returned. After a read call returns having successfully read some | ||
| bytes, the total number of bytes read will be updated. If that total | ||
| is equal to the size of the buffer, read will return successfully. | ||
|
|
||
| # Drawbacks | ||
|
|
||
| The major weakness of this API (shared with write_all) is that in the | ||
| event of an error, there is no way to return the number of bytes that | ||
| were successfully read before the error. But since that is the design | ||
| of write_all, it makes sense to mimic that design decision for read_all. | ||
|
|
||
| # Alternatives | ||
|
|
||
| One alternative design would return some new kind of Result which | ||
| could report the number of bytes sucessfully read before an error. | ||
| This would be inconsistent with write_all, but arguably more correct. | ||
|
|
||
| If we wanted io::Error to be a smaller type, ErrorKind::ShortRead | ||
| could be unparameterized. But this would reduce the information | ||
| available to calleres. | ||
|
|
||
| Finally, in the event of a short read, we could return Ok(number of | ||
| bytes read before EOF) instead of an error. But then every user would | ||
| have to check for this case. And it would be inconsistent with | ||
| write_all. | ||
|
|
||
| Or we could leave this out, and let every Rust user write their own | ||
| read_all function -- like savages. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unnecessary. Cargo makes us a highly developed distributed culture. With minimal investment needed to contribute & make a difference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If you mean we don't need a standard library because we have Cargo then I disagree strongly. Putting things off into crates was a strategy used to reach a stable 1.0 library in time, but the standard library should catch up eventually. If you do a thing, you should do it good. If standard library does reading and writing, then it should cover such basics as filling an entire buffer in face of interrupts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ArtemGr Completely agree There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm disagreeing with saying we're savages if it's not in libstd. And we don't have to write it ourselves -- it's easy to reuse code using cargo, very easy. Not saying that's a replacement but an augmentation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not savages, but babies. = ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why you do this, calling people names is beside the point of the RFC and doesn't help. I'll reply with some on-topic questions in the main discussion.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huuuuuge -1 for 'like savages' here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry it didn't help, @bluss. I had to try. = } There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So ignoring the last line of this RFC. As has already been pointed out there is a discrepancy between reading/writing with the std::io as it currently stands. I comments on here would also reenforce that its already catching a number of us out. Simply deferring to cargo for a single function will also create problems. Not only in the cost of tracking down the correct crate (maybe byteorder in this case?) but also the cost of pull in all of its dependencies etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The crate can be designed for that use case though: small and without dependencies. Using cargo is super easy IMO. Since crates may even re-export other crates, you can even ask authors to split out features to sub crates (when it's possible). |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would note that having a parameterized
ErrorKind::ShortReadwouldn't actually increase the size ofio::Error, only that ofErrorKind.