RFR: 8341594: Use Unsafe to coalesce reads in java.util.zip.ZipUtils [v2]

Claes Redestad redestad at openjdk.org
Sun Oct 6 16:15:36 UTC 2024


On Sun, 6 Oct 2024 14:56:27 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

>> Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   copyright
>
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1644:
> 
>> 1642:                 // Now scan the block backwards for END header signature
>> 1643:                 for (int i = buf.length - ENDHDR; i >= 0; i--) {
>> 1644:                     if (get32(buf, i) == ENDSIG) {
> 
> Maybe a matter of personal preference, but I think `GETSIG(buf, i) == ENDSIG` reads better than `get32(buf, i) == ENDSIG`.
> 
> The fact that it's 32 bits is kind of a detail and it doesn't reveal as well that we intend to read a signature.
> 
> So could we keep GETSIG, but add an index? There are places in `ZipInputStream` as well which could make use of that for signature checking. (But maybe not for this PR)
> 
> Alternatively, `ENDSIG(buf, i) == ENDSIG` would be consistent with `CENSIG(buf, i)` uses. 
> 
> Same applies to the other GETSIG replacements in this file.

I think all the `GETSIG(byte[])` methods are quite nasty, and it's all used very inconsistently. I wouldn't mind going the other way and removing _all_ the `CENSIG`, `LOCNAM` etc methods and just call `get16/32/32S/64(buf, <field name>)` as appropriate.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21377#discussion_r1789151311


More information about the core-libs-dev mailing list