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

[Remi Forax]

Hi Steve,
do you have checked that the your patch doesn't load a bunch of supplementary classes at start-up ?
because JarFile is used at start-up, it triggers the load of Runtime.Version and Runtime.Version has a dependency on a dozen of classes in java.util.regex.

cheers,
Rémi

----- Mail original -----
> De: "Steve Drach" <steve.drach at oracle.com>
> À: "Joseph D. Darcy" <joe.darcy at oracle.com>
> Cc: "Core-Libs-Dev" <core-libs-dev at openjdk.java.net>
> Envoyé: Jeudi 16 Juin 2016 23:44:14
> Objet: [SPAM-RENATER]Re: RFR: 8150680 JarFile.Release enum needs reconsideration with	respect to it's values
> 
> 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/>
>  
> > 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.
> > 
> 
>