On Thu, 13 Mar 2025 19:31:31 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add code comment
>
> test/jdk/java/util/zip/ZipFile/ZipFileSharedSourceTest.java line 78:
>
>> 76:
>> 77: /**
>> 78: * Creates multiple instances of java.util.zip.ZipFile with the
>> given {@code entryNameCharset}
>
> This comment could be interpreted as saying that the construction of the
> ZipFile instances happens up-front, on the main test thread. However, looking
> at the actual code, the construction of the ZipFile instance AND the
> enumeration happens concurrently.
>
> Perhaps we could trim the "how" part here, and instad just say that the test
> verifies that concurrent construction and enumeration of
> java.util.zip.ZipFile instances backed by a the same ZIP file does not cause
> unexpected encoding-related concurrency failures?
I've now updated the test method comment to clarify what the test does.
> test/jdk/java/util/zip/ZipFile/ZipFileSharedSourceTest.java line 86:
>
>> 84: @ParameterizedTest
>> 85: @MethodSource("charsets")
>> 86: void testMultipleZipFileInstances(final Charset
>> entryNameCommentCharset) throws Exception {
>
> The Charset parameter could be renamed to `charset`.
Done.
> test/jdk/java/util/zip/ZipFile/ZipFileSharedSourceTest.java line 111:
>
>> 109: private final CountDownLatch startLatch;
>> 110:
>> 111: private ZipEntryIteratingTask(final Path file, final Charset
>> entryNameCharset,
>
> The Charset parameter/field could be renamed to `charset`.
Done.
> test/jdk/java/util/zip/ZipFile/ZipFileSharedSourceTest.java line 127:
>
>> 125: try (final ZipFile zf = new ZipFile(this.file.toFile(),
>> this.entryNameCharset)) {
>> 126: final var entries = zf.entries();
>> 127: while (entries.hasMoreElements()) {
>
> I think it would be good to include a call to `ZipFile::getEntry` here as
> well, since that exercises a separate code path.
Done. The updated PR now does a `ZipFile.getEntry()` call too.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2003489767
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2003491140
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2003490799
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2003490505
