RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v9]

[Eirik Bjorsnos]

On Mon, 13 Feb 2023 20:00:51 GMT, Lance Andersen <lancea at 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