On Sun, Apr 12, 2020 at 4:18 PM Claes Redestad <claes.redes...@oracle.com>
wrote:
> I think this is shaping up nicely.
>
> If we ensure the array returned from getMetaInfVersions is already
> sorted (by e.g. using a TreeMap during parse), we could remove the
> stream expression. And if we check bounds in getVersionedEntry we don't
> need the field in JarFile at all.
>
Yeah, that extra field is kind of redundant. I guess my thought was that
ordering semantics of MRJ belongs in JarFile, but it's also great to reduce
duplicated state. Perhaps we could update the ZipFile.metadataVersions
field comment to say "ordered list"?
> In initCEN I think you need to subtract 9 from nlen:
>
> getMetaVersion(cen, pos + CENHDR + 9, nlen - 9)
>
Nice catch. The constant 9 feels a bit magic here, so maybe we could add a
comment saying // 9 is "META-INF/".length. (And / or extract a local
variable)
> And in getMetaVersion I think we can avoid creating Strings and parse
> the version inline. By using 0 as the "not a version" value we can
> simplify it further:
>
> int version = 0;
> for (int i = vstart; i < nend; i++) {
> final byte c = name[i];
> if (c == '/') {
> return version;
> }
> if (c < '0' || c > '9') {
> return 0;
> }
> version = version * 10 + c - '0';
> // Check for overflow
> if (version < 0) {
> return 0;
> }
> }
>
This is brilliantly clever. I stopped trying to be clever at some point in
order to maintain readability, but I think this code is both readable _and_
clever.
The javadocs for getMetaVersion should be updated to specify "Otherwise,
return 0"
Eirik.
