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

BufRead::read_line is not as described in the io RFC #22588

Closed
mahkoh opened this issue Feb 20, 2015 · 13 comments
Closed

BufRead::read_line is not as described in the io RFC #22588

mahkoh opened this issue Feb 20, 2015 · 13 comments

Comments

@mahkoh
Copy link
Contributor

@mahkoh mahkoh commented Feb 20, 2015

#![feature(io)]

use std::io::{BufRead};

fn main() {
    let buf = &[][];

    let mut reader = std::old_io::BufReader::new(buf);
    println!("{:?}", reader.read_line());

    let mut reader = std::io::BufReader::new(buf);
    let mut dummy = String::new();
    println!("{:?}", reader.read_line(&mut dummy));
}
Err(IoError { kind: EndOfFile, desc: "end of file", detail: None })
Ok(())

The read_until and read_line methods are changed to take explicit, mutable buffers, for similar reasons to read_to_end. (Note that buffer reuse is particularly common for read_line). These functions include the delimiters in the strings they produce, both for easy cross-platform compatibility (in the case of read_line) and for ease in copying data without loss (in particular, distinguishing whether the last line included a final delimiter).

In particular: No changes to the error semantics were mentioned in the RFC.

@aturon
Copy link
Member

@aturon aturon commented Feb 20, 2015

@aturon
Copy link
Member

@aturon aturon commented Feb 20, 2015

Nominating for 1.0-beta P-backcompat-libs I-needsdecision

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Feb 23, 2015

I'm not sure that a change is necessitated here. When EOF is hit then no data is pushed onto the provided buffer, so if the buffer is of the same length as before and Ok(()) is returned then you know the stream is at EOF?

@mahkoh
Copy link
Contributor Author

@mahkoh mahkoh commented Feb 23, 2015

That was not part of the RFC.

@tfogal
Copy link

@tfogal tfogal commented Feb 23, 2015

Reading the RFC now, I see:

The EndOfFile variant will be removed in favor of returning Ok(0) from read on end of file (or
write on an empty slice for example). This approach clarifies the meaning of the return value of
read, matches Posix APIs, and makes it easier to use try! in the case that a "real" error should be
bubbled out.

POSIX APIs (i.e. getline from POSIX 2008) return EOF if the previous call exhausted the stream, not 0. I get why try! makes the situation in Rust unique, but it's really unintuitive to have read-esque calls that cannot possibly produce an EOF (why have the concept of EOF at all?).

I think the RFC should be updated. To meet in the middle (still work with try!), perhaps it could return Ok(()) at the first stream exhaustion and then Err(EOF) on all subsequent calls? Then POSIX-like looping (searching for EOF) would still work.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Feb 24, 2015

@mahkoh

That was not part of the RFC.

This is not a helpful comment. It is not possible for the RFC to spell out 100% each minute implementation detail of all proposed functions and RFCs are intentionally left with wiggle room for the implementation. It would be far more useful for you to make a case for this to return Err instead of sprinkling curt comments.


@tfogal

I think the RFC should be updated. To meet in the middle (still work with try!), perhaps it could return Ok(()) at the first stream exhaustion and then Err(EOF) on all subsequent calls? Then POSIX-like looping (searching for EOF) would still work.

I believe that the current implementation is consistent with our EOF semantics elsewhere, so an amendment such as this would need an RFC PR of its own. Would you be interested in doing so?

@mahkoh
Copy link
Contributor Author

@mahkoh mahkoh commented Feb 24, 2015

It is not possible for the RFC to spell out 100% each minute implementation detail of all proposed functions and RFCs are intentionally left with wiggle room for the implementation.

Changing the interface of read_line from

while let Ok(s) = buf.read_line() {
    // code
}

to

let mut s = String::new();
while buf.read_line(&mut s).is_ok() {
    if s.len() == 0 {
        break;
    }
    // code
    s.truncate(0);
}

Is not a "minute implementation detail".

It would be far more useful for you to make a case for this to return Err instead of sprinkling curt comments.

I already discussed read_line and lines with @aturon during the RFC process. This was not part of our discussion.

@mahkoh
Copy link
Contributor Author

@mahkoh mahkoh commented Feb 24, 2015

s.truncate(0);

That looks a bit dangerous because Rust doesn't have proper for loops. Maybe more like this:

let mut s = String::new();
while { s.truncate(0); buf.read_line(&mut s).is_ok() && s.len() > 0 } {
}
@tfogal
Copy link

@tfogal tfogal commented Feb 24, 2015

@alexcrichton I started down that road but it got painful and raised more questions (like why doesn't this take some kind of iterator?). Comments welcome over at rust-lang/rfcs#903.

@mahkoh
Copy link
Contributor Author

@mahkoh mahkoh commented Feb 24, 2015

This could be fixed by having read_line return the number of bytes read and then

#[derive(Copy)]
enum Error {
    EndOfFile = 0,
    WouldBlock = -1,
    // etc.
}

trait ToIsize {
    fn to_isize(&self) -> isize;
}

impl ToIsize for Result<usize, Error> {
    fn to_isize(&self) -> isize {
        match *self {
            Ok(u) => u as isize,
            Err(e) => e as isize,
        }
    }
}
while buf.read_line(&mut s).to_isize() > 0 {
    // code
}
@mahkoh
Copy link
Contributor Author

@mahkoh mahkoh commented Feb 24, 2015

Note that Rust can't have u8 slices larger than isize::MAX so there is no problem with overflow.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Feb 24, 2015

I'm going to close this in favor of the now-open RFC, thanks @tfogal!

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Feb 24, 2015

er, rfc link: rust-lang/rfcs#903

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants