A relatively minor update. I simplified VersionHelper. No other changes. http://cr.openjdk.java.net/~sdrach/8153654/webrev.12/ <http://cr.openjdk.java.net/~sdrach/8153654/webrev.12/>
> On Sep 14, 2016, at 4:06 PM, Steve Drach <steve.dr...@oracle.com> wrote: > > 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 >
