Hi. I would like to point out that it appears JarFile.Release enum is specifically tailored to address multi-version jar specification. However, I find nothing in that enum specific except for the documentation and BASE_VERSION. Wouldn't it better to create a top-level Release enum that can be used to identify anything in the JDK with release semantics -- apart from Jar files?
Cheers, Paul On Tue, Jun 21, 2016 at 1:23 PM, Steve Drach <steve.dr...@oracle.com> wrote: > Hi Paul, > > I believe this webrev addresses your concerns: > > http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html < > http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html> > > > > On Jun 16, 2016, at 3:49 PM, Paul Sandoz <paul.san...@oracle.com> wrote: > > > > > >> On 16 Jun 2016, at 14:44, Steve Drach <steve.dr...@oracle.com> wrote: > >> > >> This webrev uses methods instead of fields to return the base and > runtime values used internally by JarFile. I’ve also optimized it a bit. > >> > >> http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/ < > http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/> > >> > > > > JarFIle > > — > > > > 132 private final static int base_version; > > > > You are using lower case, here, this caught me out as i thought it was > an non-static field. Call it something like BASE_VERSION_MAJOR. > > > > > > 155 BASE_VERSION = > Runtime.Version.parse(String.valueOf(base_version)); > > > > 164 RUNTIME_VERSION = > Runtime.Version.parse(String.valueOf(runtimeVersion)); > > > > Use Integer.toString rather than String.valueOf (also update > specification). > > > > > > 337 public final Runtime.Version getVersion() { > > 338 if (VERSION == null) { > > 339 if (isMultiRelease()) { > > 340 VERSION = > Runtime.Version.parse(String.valueOf(version)); > > 341 } else { > > 342 VERSION = BASE_VERSION; > > 343 } > > 344 } > > 345 return VERSION; > > 346 } > > 347 private Runtime.Version VERSION; > > > > You are using the style for a static field. > > > > In the JarFile constructor why don’t you just store the version passed > in unless MULTI_RELEASE_FORCED? > > > > Have final fields: > > > > final Runtime.Version version; > > final int version_major; > > > > then do: > > > > if (MULTI_RELEASE_FORCED || version.major() == RUNTIME_VERSION.major()) > { > > // This also deals with the common case where the value from > JarFile.runtimeVersion() is passed > > this.version = RUNTIME_VERSION; > > } else if (version.major() <= BASE_VERSION_MAJOR) { > > // This also deals with the common case where the value from > JarFile.baseVersion() is passed > > this.version = BASE_VERSION; > > } else { > > // Canonicalize > > this.version = > Runtime.Version.parse(Integer.toString(version.major())); > > } > > this.version_major = version.major(); > > > > Paul. > > > > > > > > > >>> On Jun 15, 2016, at 4:31 PM, Joseph D. Darcy <joe.da...@oracle.com> > wrote: > >>> > >>> Steve, > >>> > >>> In JarFile, please use methods not fields to return the new > information. The information in question is not constant across versions. > Using methods instead of fields avoid over-committing on a particular > implementation, etc. > >>> > >>> Cheers, > >>> > >>> -Joe > >>> > >>> On 6/15/2016 3:49 PM, Steve Drach wrote: > >>>> I’ve updated the webrev to address the issue of the constructor > accepting values like Version.parse(“7.1”) > >>>> > >>>> http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/ < > http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/> > >>>> > >>>>> On Jun 15, 2016, at 8:56 AM, Steve Drach <steve.dr...@oracle.com> > wrote: > >>>>> > >>>>>>> Please review the following changeset: > >>>>>>> > >>>>>>> webrev: > http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html < > http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html> > >>>>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 < > https://bugs.openjdk.java.net/browse/JDK-8150680> > >>>>>>> > >>>>>>> The issue calls for reconsidering the JarFile.Release enum. A > comment in the bug report suggests replacing JarFile.Release with > Runtime.Version, and that’s what I did. Specifically I removed the enum, > changed the constructor to accept a Runtime.Version object instead of a > JarFile.Release object, updated all places in the JDK that invoked the > constructor and updated all tests. > >>>>>>> > >>>>>> Moving to Runtime.Version seems right but doesn't the javadoc for > the constructor need to be updated to make it clear how it behavior when > invoking with something like Version.parse("7.1") ? If I read the code > correctly then this will be accepted and getVersion() will return 7.1. > >>>>> Yes, it needs to be updated and it needs to be fixed. Thanks for > finding that. > >>>>> > >>>>>> Fields or methods is another discussion point for the base and > runtime versions. > >>>>> My thinking is, in this case fields and methods are equivalent, the > method not giving any more flexibility than a field. For example the > method JarFile.baseVersion will just return the value contained in the > private final static field BASE_VERSION. Or the public final static field > BASE_VERSION can be directly accessed. I see no advantage of a method > here. But I’m willing to be enlightened. > >>>>> > >>>>>> -Alan. > >>> > >> > > > >
