RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Paul Benedict]

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.drach at 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.sandoz at oracle.com> wrote:
> >
> >
> >> On 16 Jun 2016, at 14:44, Steve Drach <steve.drach at 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.darcy at 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.drach at 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.
> >>>
> >>
> >
>
>