Hi Paul, > Hi. I would like to point out that it appears JarFile.Release enum is > specifically tailored to address multi-version jar specification.
I think you are looking at the current code, not the webrev. What the webrev does is remove the JarFile.Release enum. > 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 > <mailto: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> > <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 > > <mailto:paul.san...@oracle.com>> wrote: > > > > > >> On 16 Jun 2016, at 14:44, Steve Drach <steve.dr...@oracle.com > >> <mailto: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/> > >> <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 > >>> <mailto: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/> > >>>> <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 > >>>>> <mailto: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> > >>>>>>> <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> > >>>>>>> <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. > >>> > >> > > > >
