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

[Paul Sandoz]

> On 23 Jun 2016, at 04:04, Steve Drach  wrote:
> 
>> 144 private boolean notVersioned;   // legacy constructor called
>> 
>> Do you need this?
> 
> Unfortunately yes.  It’s used in entries() and stream().  If it’s set, they 
> have the JDK 8 semantics.  If not set, entries/stream only see the 
> appropriate versioned entries.

Ah, i see it just strips out the resources under the versioned area.


>  This will go away when JDK-8157524 is resolved.
> 

Ok.

Paul.

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

[Steve Drach]

> 144 private boolean notVersioned;   // legacy constructor called
> 
> Do you need this?

Unfortunately yes.  It’s used in entries() and stream().  If it’s set, they 
have the JDK 8 semantics.  If not set, entries/stream only see the appropriate 
versioned entries.  This will go away when JDK-8157524 is resolved.

> Is this condition already met if versionMajor == BASE_VERSION_MAJOR ?

No

> 
> i.e. why do you need to restrict to only the “legacy” constructor, since 
> surely the same conditions should apply if a BASE_VERSION compatible version 
> is passed into the version accepting constructor?

It’s the only way to differentiate how the version accepting constructor was 
called, from another constructor or from the “outside” world.

> 
> Paul.

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

[Paul Sandoz]

> On 21 Jun 2016, at 20:23, Steve Drach  wrote:
> 
> Hi Paul,
> 
> I believe this webrev addresses your concerns:
> 
> http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html
> 

 141 private final int base_version; // BASE_VERSION.major()

Why is this an instance field? make static and rename to BASE_VERSION_MAJOR.


 142 private final Runtime.Version version;  // current version

 143 private final int major_version;// version.major()

rename to versionMajor


 144 private boolean notVersioned;   // legacy constructor called

Do you need this? Is this condition already met if versionMajor == 
BASE_VERSION_MAJOR ?

i.e. why do you need to restrict to only the “legacy” constructor, since surely 
the same conditions should apply if a BASE_VERSION compatible version is passed 
into the version accepting constructor?

Paul.

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

[Steve Drach]

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  > wrote:
> Hi Paul,
> 
> I believe this webrev addresses your concerns:
> 
> http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html 
>  
>  >
> 
> 
> > On Jun 16, 2016, at 3:49 PM, Paul Sandoz  > > wrote:
> >
> >
> >> On 16 Jun 2016, at 14:44, Steve Drach  >> > 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/ 
> >>  
> >>  >> >
> >>
> >
> > 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  >>> > 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/ 
>   
>    >
> 
> > On Jun 15, 2016, at 8:56 AM, Steve Drach  > > wrote:
> >
> >>> Please review the following changeset:
> >>>
> >>> webrev: 
> >>> http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
> >>>  
> >>>  >>> >
> >>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 
> >>>  
> >>>  >>> 

Re: 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  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  wrote:
> >
> >
> >> On 16 Jun 2016, at 14:44, Steve Drach  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 
> 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 
> 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 e

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

[Steve Drach]

Hi Paul,

I believe this webrev addresses your concerns:

http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html 



> On Jun 16, 2016, at 3:49 PM, Paul Sandoz  wrote:
> 
> 
>> On 16 Jun 2016, at 14:44, Steve Drach  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/ 
>> 
>> 
> 
> 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  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/ 
 
 
> On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:
> 
>>> Please review the following changeset:
>>> 
>>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
>>> 
>>> issue: 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.
>>> 
>> 
> 

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

[Steve Drach]

Hi Remi,

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

Is it still true, in JDK 9, that JarFile is loaded at start-up?  Using a simple 
Hello World program, I started it with the option -verbose:class and did not 
see either JarFile or Version loaded.  Just to verify, I added a print 
statement to a static initializer in both JarFile and Version, and again did 
not see either class loaded.  I also tried the tests with a jar file on the 
class path and did see both classes loaded.

> 
> cheers,
> Rémi
> 
> - Mail original -
>> De: "Steve Drach" 
>> À: "Joseph D. Darcy" 
>> Cc: "Core-Libs-Dev" 
>> Envoyé: Jeudi 16 Juin 2016 23:44:14
>> Objet: [SPAM-RENATER]Re: RFR: 8150680 JarFile.Release enum needs 
>> reconsideration withrespect 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  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  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.
>>> 
>> 
>> 

Re: 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" 
> À: "Joseph D. Darcy" 
> Cc: "Core-Libs-Dev" 
> 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  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  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.
> > 
> 
> 

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

[Paul Sandoz]

> On 16 Jun 2016, at 14:44, Steve Drach  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/ 
> 
> 

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  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/ 
>>> 
>>> 
 On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:
 
>> Please review the following changeset:
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
>> 
>> issue: 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.
>> 
> 

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

[Steve Drach]

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/ 

 
> On Jun 15, 2016, at 4:31 PM, Joseph D. Darcy  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/ 
>> 
>> 
>>> On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:
>>> 
> Please review the following changeset:
> 
> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
> 
> issue: 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.
> 

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

[Joseph D. Darcy]

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/ 



On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:


Please review the following changeset:

webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 

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

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

[Steve Drach]

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/ 


> On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:
> 
>>> Please review the following changeset:
>>> 
>>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
>>> 
>>> issue: 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.
> 

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

[Peter Levart]

Hi Steve,


On 06/15/2016 05:56 PM, Steve Drach wrote:

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



If you make the static final field part of public API, then you 
constrain yourself to initializing it in class initializer. If you keep 
it private and expose it via a static method now, then you keep the 
freedom to initialize it lazily if it ever happens to be needed in the 
future because of various reasons such as initialization circularity. 
But I don't know if this applies in your case. Would you ever need to 
use part of JarFile class early in bootup sequence before those fields 
can be initialized?


Regards, Peter

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

[Steve Drach]

>> Please review the following changeset:
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
>> 
>> issue: 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.

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

[Alan Bateman]

On 15/06/2016 00:17, Steve Drach wrote:

Hi,

Please review the following changeset:

webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 

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


Fields or methods is another discussion point for the base and runtime 
versions.


-Alan.

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

[Steve Drach]

Hi,

Please review the following changeset:

webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 

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

Thanks,
Steve