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. >>> >> >
