Thanks Alan and Mandy for the review. I am guessing the Alan’s preference to use Files.write(aFilePath, lines) is to avoid extra String.join operation, which I would too. The current version with byte[] is more flexible but we most likely won't need it in launcher. Anyway, I don’t think the difference would be noticeable with launcher tests.
The updated webrev[1] takes a boolean to decide which Files.write to use but gives up the byte[] version. Cheers, Henry [1] http://cr.openjdk.java.net/~henryjen/jdk/8231863.1/webrev/ > On Nov 9, 2019, at 11:33 AM, Mandy Chung <mandy.ch...@oracle.com> wrote: > > The patch looks fine to me as well. > > As for the test, perhaps adding a new createAFile(File aFile, List<String> > lines, boolean hasTrailingBlankLine) in TestHelper instead may help avoid any > confusion. > > > Mandy > > On 11/8/19 7:43 AM, Mat Carter wrote: >> Hi Alan >> >> The method you propose: [nio.]Files[.write](aFile.toPath, lines) adds a >> trailing blank line to the file; the regression test needs to generate a >> file without a trailing blank line as this is the condition in which the bug >> occurs. This is why it now writes out an array of bytes >> >> Cheers >> Mat >> >> ________________________________ >> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of >> Alan Bateman <alan.bate...@oracle.com> >> Sent: Friday, November 8, 2019 2:56 AM >> To: Henry Jen <henry....@oracle.com>; core-libs-dev@openjdk.java.net >> <core-libs-dev@openjdk.java.net> >> Subject: Re: RFR: 8231863: Crash if classpath is read from @argument file >> and the main gets option argument >> >> On 07/11/2019 22:55, Henry Jen wrote: >>> Hi, >>> >>> Please review the webrev[1], contributed by Mat Carter. You can find the >>> bug details at JBS[2]. I have reviewed and tested the fix, I still need an >>> official review before I can push this. >>> >> Looks okay although in the test, the createAFile helper method could be >> replaced with Files(aFile.toPath, lines) and that would avoid the need >> to concatenate all the lines. You can specify the defaultCharset to that >> method if you need really it. >> >> -Alan >
