On Mon, 24 Mar 2025 12:52:49 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - improve code comment for ZipFile.zipCoder
>> - Alan's suggestion - change code comment about Source class being thread
>> safe
>> - Alan's suggestion - trim the javadoc of (internal) ZipCoder class
>
> src/java.base/share/classes/java/util/zip/ZipCoder.java line 181:
>
>> 179:
>> 180: /**
>> 181: * {@return the {@link Charset} used this {@code ZipCoder}}
>
> Suggestion:
>
> * {@return the {@link Charset} used by this {@code ZipCoder}}
Fixed.
> src/java.base/share/classes/java/util/zip/ZipFile.java line 85:
>
>> 83: private final String filePath; // ZIP file path
>> 84: private final String fileName; // name of the file
>> 85: // ZipCoder for entry names and comments when not using UTF-8
>
> "_when not using UTF-8_" could be confusing.
>
> The ZipCoder here may well be UTF-8, it's more about the entry not mandating
> UTF-8 by having its language encoding flag set.
>
> I think we should either clearly explain when this ZipCoder is used (when
> entries do not mandate UTF-8), or drop the information here and lean on it
> being explained in `zipCoderFor`.
>
> If we decide to drop it, this could be just:
>
> `// Used when decoding entry names and comments`
>
> If we decide to keep it, then something like:
>
> `// Used when decoding entry names and comments, unless entry flags mandate
> UTF-8`
I used your suggestion of "Used when decoding entry names and comments" to keep
it simple.
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1145:
>
>> 1143: static record EntryPos(String name, int pos) {}
>> 1144:
>> 1145: // Implementation note: This class is be thread safe.
>
> Suggestion:
>
> // Implementation note: This class is thread safe.
Fixed
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1432:
>
>> 1430: * where a ZIP file is re-opened after it has been modified).
>> 1431: * - The Charset, that was provided when constructing a
>> ZipFile instance,
>> 1432: * for reading non-UTF-8 entry names and comments.
>
> I think it would be sufficient to say "The Charset that was provided when
> constructing the ZipFile instance". Any non-UTF8-ness is better explained
> elsewhere.
Fixed
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1438:
>
>> 1436: private final BasicFileAttributes attrs;
>> 1437: private final File file;
>> 1438: // the Charset to be used for processing non-UTF-8 entry
>> names in the ZIP file
>
> Similarly this could just say "The Charset that was provided when
> constructing the ZipFile instance"
Yes, I think your suggested comment is good. I've updated the PR to use this.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068530939
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068531394
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068531636
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068531991
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068533150
