The most recent webrev is http://cr.openjdk.java.net/~sdrach/8153654/webrev.11/ <http://cr.openjdk.java.net/~sdrach/8153654/webrev.11/>
Comments inline >> The latest web revs. Answers to questions in-line below: >> >> http://cr.openjdk.java.net/~sdrach/8153654/webrev.10/ >> >> http://cr.openjdk.java.net/~sdrach/8153654/webrev.jdk/ > > This looks pretty good. My main comment is in VersionHelper (see below). > > ClassFileReader.java > > 337 String[] msg = {"err.multirelease.option.exists", > f.getName()}; > 389 String[] msg = > {"err.multirelease.jar.malformed”, > 390 jarfile.getName(), rn > 391 }; > > `msg` is not used. These lines can be removed. Done > > line 81-95: this wants to add to VersionHelper if it’s a versioned entry. I > suggest to move this to VersionHelper, something like this and replace the > add method. I did essentially the same thing, but not exactly the same. > > public static void addIfVersionedEntry(Path jarfile, String realEntryName, > String className) { > if (realEntryName.startsWith(META_INF_VERSIONS)) { > int len = META_INF_VERSIONS.length(); > int n = realEntryName.indexOf('/', len); > if (n > 0) { > String version = realEntryName.substring(len, n); > assert (Integer.parseInt(version) > 8); > > String v = versionMap.get(className); > if (v != null && !v.equals(version)) { > // name associated with a different ClassFile > throw new > MultiReleaseException("err.multirelease.version.associated", className, > v, version); > } > versionMap.put(className, version); > } else { > throw new MultiReleaseException("err.multirelease.jar.malformed", > jarfile.toString(), realEntryName); > } > } > } > > I prefer to call String::length rather than hardcoding the length. OK > I don’t see why VersionHelper caches ClassFile. A JarEntry (base) name has a one to many relationship with versions. This implies the class name also has a one to many relationship with versions. But a ClassFile (derived from the contents of the JarEntry) has a one to one relationship with version. We desire a one to one relationship for className to version and one way to assure that is to create two maps as I now have done. I think the code is more obvious now, at least I hope it is. > I think it can cache a map from class name to the version for now. We may > have to handle duplicated classes in more than one modular jar (it’s not > exported and should be allowed). We could file a separate issue to look into > later. > > This needs a CCC for the new --multi-release option. That’s next. > > Mandy
