smol-rs / futures-lite

Futures, streams, and async I/O combinators.
Apache License 2.0
449 stars 26 forks source link

Append to string buffer when calling read_line with string buffer #60

Open oysols opened 2 years ago

oysols commented 2 years ago

This matches implementation of futures-rs and behavior of BufRead::read_line. This should also fix issue of panic and "drops 6 characters" mentioned in #47.

fogti commented 1 year ago

wouldn't it be more appropriate to do this in https://github.com/smol-rs/futures-lite/blob/a28ac5b24fbd6f5a3b45a41488d65b8d8e5d1f1b/src/io.rs#L1697-L1710 and replace that segment by

    match core::str::from_utf8(&bytes[..]) {
        Ok(s) => {
            // idk what the following line wants to accomplish exactly...
            debug_assert_eq!(*read, 0);
            *buf += s;
            bytes.clear();
            Poll::Ready(ret)
        }
        Err(_) => Poll::Ready(ret.and_then(|_| {
            Err(Error::new(
                ErrorKind::InvalidData,
                "stream did not contain valid UTF-8",
            ))
        })),
    }

this gets rid of the requirement that the buffer needs to be empty initially, and avoids an unnecessary string allocation in case the buffer already has content. on the other hand, it has the disadvantage that in the arguably common case that the buffer is empty, it does an unnecessary allocation (a few times for bytes and once (this is added) for buf); the approach of this PR does have that disadvantage in the "alternative case" that the buffer is already filled (it discards the originally allocated buffer, and duplicates it into bytes). Optimizing for the "alternative case" might be a good idea if someone wants to append to a buffer which is already filled with a few MiB of string data, because we avoid re-doing the UTF-8 validation of the original buffer contents in that case. Deciding between these approaches probably requires benchmarking both.