On Mon, 13 Feb 2023 20:00:51 GMT, Lance Andersen <lan...@openjdk.org> wrote:
>> Eirik Bjorsnos has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Revert accidental removal of UTF8ZipCoder.compare
>
> src/java.base/share/classes/java/util/zip/ZipCoder.java line 66:
>
>> 64: NO_MATCH
>> 65: }
>> 66:
>
> Please add a comment indicating what the values mean
I added comments to the enum and each of the values.
> src/java.base/share/classes/java/util/zip/ZipCoder.java line 210:
>
>> 208: * is known and matches the charset of this ZipCoder.
>> 209: */
>> 210: Comparison compare(String str, byte[] b, int off, int len, boolean
>> addSlash) {
>
> If you could add an `@param` comments, that would be awesome 😎
I improved the Javadoc of this method and added `@param` comments.
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1665:
>
>> 1663: }
>> 1664: // No exact match found, will return either slashMatch or
>> -1
>> 1665: return slashMatch;
>
> This gets a bit confusing as we return `pos` when we have an exact match so
> it would be helpful to had more clarity via additional comments(it might not
> have been clear with the previous comments but I think if we are going to add
> `slashMatch` we should take the time to beef up the comments
The dual-modality of this method certainly allows for some head-scratch trying
to find a succinct way to describe its logic. I have made an attempt to improve
it, but I'm sure it could be even better.
The `slashPos` name was probably ok as a local variable, but now that it is
part of the contract of ZipCoder.compare, I think it helps to rename the enum
value to `DIRECTORY_NAME` and update `slashPos` to `dirPos` accordingly.
Do you have any suggestions on how to improve the comments in the last version?
> test/jdk/java/util/zip/ZipFile/TestZipFileEncodings.java line 127:
>
>> 125: }
>> 126:
>> 127: @Test
>
> Please add a comment introducing the test
I described the rationale of adding this test in a comment.
> test/jdk/java/util/zip/ZipFile/TestZipFileEncodings.java line 143:
>
>> 141: }
>> 142: }
>> 143: @Test(dataProvider = "all-charsets")
>
> Please add a comment introducing the test
I described the rationale of adding this test in a comment.
> test/jdk/java/util/zip/ZipFile/TestZipFileEncodings.java line 307:
>
>> 305: ze.setCrc(crc.getValue());
>> 306: }
>> 307: ze.setTime(1675862371399L);
>
> Please add a comment indicating what the time is
This change was accidentaly introduced, I have reverted it. Good catch.
-------------
PR: https://git.openjdk.org/jdk/pull/12290
