On Sun, 6 Oct 2024 14:56:27 GMT, Eirik Bjørsnøs <eir...@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
