Would it be possible to send an updated webrev with the patch that is
proposed for jdk/jdk? One thing that I'm concerned about is that
META-INF/* is a JAR file concepts and I'd prefer not build up further
shared secrets that operate on ZipFile (should be consistent JarFile and
JarEntry when dealing with JAR files).
-Alan
On 12/04/2020 20:06, Eirik Bjørsnøs wrote:
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.
