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