Hi Steve,
Here are my observations. I haven't followed the development of
multi-release jar files very closely, so I might be missing some quirks,
but I speak here from the knowledge I get from the javadocs...
Javadoc for the new method mentions public and non-public classes:
"@{code JarEntries}
* containing non-public classes in the
* versioned directory that corresponds to the version returned by
* {@link JarFile#getVersion()} are included in the stream because
they are
* typically accessed by the public classes."
Why? The logic of multi-release jar files is indifferent towards the
access attributes of class files. It never parses the class file to
interpret the access attributes of the containing class. The class-level
javadoc of JarFile does mention "public classes", but I view this just
as a recommendation how they should be constructed.
The logic of multi-release jar files is indifferent towards the types of
resources contained in the jar entries too, as far as I can see. Your
logic in versionedStream() is not. The proposed new method is the only
place in JarFile and ZipFile where the check for ".class" suffix of the
entry name is performed. Why this special treatement of class files? I
also don't understand why the versioned stream should contain the
untranslated base versioned directories. For example, the following
entries for the v10 stream in the test:
"META-INF/versions/9/"
"META-INF/versions/10/"
All other resources and sub-directories in the META-INF/versions/XX/...
directories are "normalized" (meaning that the versioned prefix is
stripped from the names in the returned entries). Normalizing above
directories would translate them to "root" directory, which is never
included as an entry of a normal jar or zip file. So I think they could
simply be skipped in the resulting versioned stream. Couldn't they?
Considering all of the above, I think versionedStream() method could be
much simpler. For example, something like the following:
public Stream<JarEntry> versionedStream() {
return
!isMultiRelease()
? stream()
: stream()
.flatMap(je -> {
String name = je.getName();
if (name.startsWith(META_INF_VERSIONS)) {
// versioned entry
if (name.length() > META_INF_VERSIONS.length()) {
// potentially real entry
String vStr =
name.substring(META_INF_VERSIONS.length());
int offset = vStr.indexOf('/');
int version = 0;
if (offset >= 0) try {
version =
Integer.parseInt(vStr.substring(0, offset));
} catch (NumberFormatException e) { /*
ignore */ }
if (version <= 0) {
throw new RuntimeException(
"Can't obtain version from
multi-release jar entry: "
+ name);
}
return (version <= versionMajor &&
vStr.length() > offset + 1)
// normalized name of real entry
? Stream.of(vStr.substring(offset + 1))
// version > versionMajor or
// META-INF/versions/<version>/ dir
- ignored
: Stream.empty();
} else {
// META-INF/versions/ dir - ignored
return Stream.empty();
}
} else { // base entry
return Stream.of(name);
}
})
.distinct()
.map(this::getJarEntry);
}
What do you think?
Regards, Peter
On 08/26/2016 07:16 AM, Tagir Valeev wrote:
Hello!
Small nitpick:
versionsMap.keySet().forEach(v -> {
Stream<String> names = versionsMap.get(v).stream().map(nm -> nm.name);
if (v == versionMajor) {
// add all entries of the version we are interested in
finalNames.addAll(names.collect(Collectors.toSet()));
} else {
// add resource entries found in lower versioned directories
finalNames.addAll(names.filter(nm -> nm.endsWith(".class") ? false
: true)
.collect(Collectors.toSet()));
}
});
I suggest to replace with
versionsMap.forEach((v, list) -> {
Stream<String> names = list.stream().map(nm -> nm.name);
if (v != versionMajor) {
names = names.filter(nm -> !nm.endsWith(".class"));
}
names.forEach(finalNames::add);
});
This is cleaner and somewhat faster to use Map.forEach as you don't need to
lookup every map entry.
Also I don't see why "nm -> nm.endsWith(".class") ? false : true" could be
better than
"nm -> !nm.endsWith(".class")". Probably a matter of style though.
Finally there's no need to collect into intermediate set just to add this
set into finalNames.
Instead you can drain the stream directly to finalNames via forEach.
Probably it should be explicitly noted in spec that the resulting stream is
unordered (or at least may
be unordered) as it's created from the Set.
With best regards,
Tagir Valeev
On Fri, Aug 26, 2016 at 2:30 AM, Steve Drach <[email protected]> wrote:
Hi,
Please review this changeset that adds a versionedStream method to JarFile.
webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.00/ <
http://cr.openjdk.java.net/~sdrach/8163798/webrev.00/>
issue: https://bugs.openjdk.java.net/browse/JDK-8163798 <
https://bugs.openjdk.java.net/browse/JDK-8163798>
Thanks
Steve
