On Wed, 15 Mar 2023 22:49:57 GMT, Leonid Mesnik <lmes...@openjdk.org> wrote:
>> The StreamPumper is fixed to process the last line even it is not finishes
>> with '\n' or '\r'. The test included. Testing with tier1-3 also to verify
>> that tests are not broken.
>
> Leonid Mesnik has updated the pull request incrementally with one additional
> commit since the last revision:
>
> comments added
A few minor suggestions.
The additional commentary is very helpful. Figuring out exactly how this code
works is painful.
Thanks.
test/lib-test/jdk/test/lib/process/ProcessToolsLastLineTest.java line 50:
> 48:
> 49: // The line which exceeds internal StreamPumper buffer (256 bytes)
> 50: String VERY_LONG_LINE = "VERYLONGLINE".repeat(30);
It might be a bit more obvious that this does exceed 256 by simply doing:
String veryLongLine = "X".repeat(257);
test/lib-test/jdk/test/lib/process/ProcessToolsLastLineTest.java line 54:
> 52: System.out.print(args[0]);
> 53: } else {
> 54: test("ARG1");
Some boundary testing here would be:
test("\n");
test("\nARG1");
test("\nARG1\n");
test("ARG1/n");
test("ARG1");
test/lib/jdk/test/lib/process/StreamPumper.java line 99:
> 97: * Implements Thread.run(). Continuously read from {@code in} and
> write to
> 98: * {@code out} until {@code in} has reached end of stream.
> 99: * Additionally this method also split data read from buffer into the
> lines and process each line using linePumps.
A few grammatical nits. I suggest:
> Additionally this method also splits the data read from the buffer into
> lines, and processes each line using linePumps.
test/lib/jdk/test/lib/process/StreamPumper.java line 138:
> 136: }
> 137: // The remaining after last '\n' (lastcrlf position)
> buffer data is written into lineBos.
> 138: // The end of this line from next buffer is
> concatenated to this data in the next iteration.
In trying to help my own understanding here I suggest:
// If no crlf was found, or there was additional data after the last crlf was
found, then write the leftover data
// in lineBos. If there is more data to read it will be concatenated with the
current data on the next iteration.
// If there is no more data, or no more crlf found, all the remaining data will
be processed after the loop, as the
// final line.
test/lib/jdk/test/lib/process/StreamPumper.java line 149:
> 147: }
> 148: // Process data remaining after last '\n' in the last buffer.
> 149: // It is already written in the lineBos buffer but not
> processed because '\n' hasn't been met.
Suggestion:
// If there was no terminating crlf the remaining data has been written to
lineBos, but this final line of data
// now needs to be processed by the linePumper.
-------------
PR: https://git.openjdk.org/jdk/pull/13034
