On Apr 27, 2013, at 10:15 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> On 26/04/2013 22:59, Henry Jen wrote: >> Hi, >> >> Please review webrev at >> >> http://cr.openjdk.java.net/~henryjen/ccc/8003258.1/webrev/ >> >> It adds a method to BufferedReader. >> >> public Stream<String> lines() {} >> >> A class java.io.UncheckedIOException is also added as a general approach >> for wrapping up an IOException to be unchecked. >> >> Cheers, >> Henry >> > I'm not so sure about setting expectations that you can readily mix stream > usage with the other methods that BufferedReader defines. This puts a strict > requirement on the implementation that it must be based on readLines and that > it can never do any read ahead. > Even if the implementation uses readLines, if the stream is evaluated in parallel more lines may be read than absolutely necessary so we cannot guarantee that the following will consume at most one element: br.lines().parallel().findAny(); br.lines().parallel().findFirst(); br.lines().parallel().limit(1).collect(toList()); So we have to say something like: * <p>The reader must not be operated on during the execution of the terminal * stream operation. Otherwise, the result of the terminal stream operation is * undefined * * <p>After execution of the terminal stream operation there are no guarantees that * the reader will be at a specific position from which to read the next character or line. Paul. > The javadoc should probably specify that lines() returns a Stream even if the > reader is closed. > > Otherwise just minor comments: > > In UncheckedIOException then the @since should be 1.8. The @see probably > isn't needed here because there is already a link to IOException in the > description (but it doesn't matter). > > I agree with Stephen's comment on finally { nextLine = null; }. It might be > more obvious with a simple: > > String result = nextLine; > nextLine = null; > return result; > > The test has the Classpath exception, I assume you'll put the pure GPL header > on this before you push. > > -Alan. > > >
