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