RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

[Iris Clark]

Hi, Roger.

Thanks for the feedback.

>>      http://cr.openjdk.java.net/~iris/verona/8072379/webrev.1/

> I see the JEP says JDK specific, but does that rule out putting the version API in a Java.* package?

For now, we should avoid including this API in java.*.  If for no other reason but that we may need to evolve the API in an update release.

> Version.java:

> 229:  The IAE("Terminal...") exception may not be that easily understood; can it
> say concisely
>    that the build and optional elements are missing

I've changed it to this which is technically more accurate (but verbose):

    IAE("'+' found with neither build or optional components: ':" + s + "'").

If you have a more succinct suggestion, let me know.

> 264: current() should cache the value from the parse; it is likely to be called 
> more than once and parsing
>     an version string is a relatively expensive.

Done.

> 334: the naming of one of the version elements as 'optional' may be confusing
> because optional is an adjective.
>    Especially when the element is optional and the method named optional returns an Optional.
>   Can the name be a cogent noun. how about 'info' for informational string

The term "optional" to refer to this portion of the string is used in quite a few places, both inside and outside of OpenJDK code.  I don't think that it would be helpful to refer to this by two different names.  If you feel strongly that the name should be changed, I can file an RFE to investigate the full impact to do so.

> 396:  I'm not sure why BigInteger is needed; other than perhaps because it has a
> constructor that takes a string.

The components of $VNUM are constrained to be ints, hence if a string of numbers greater than Integer.MAX_VALUE is passed, construction of the Version will fail.  

$PRE is an arbitrary length String which may consist entirely of digits.  As long as the string of digits may be represented as a String, there is no constraint on how many digits there are.  The only time when the number of digits is a potential problem is during Version comparison.  In the case when both of the Version objects contain $PRE consisting entirely of digits, we need to consider the case where the number of digits would result in a integer larger than Integer.MAX_VALUE.

> 433:  Optional has a .ifPresent(xxx) method that could be used to streamline the
> code.
>       pre.ifPresent( v -> sb.append("-").append(v));

Done.

> OracleVersion.java:   Can this be renamed more functionally to reflect 
> that the 4'th component is a patch number.
>     It might be useful to folks other than Oracle.
> 
> 107: the constructor should be declared private since it is not needed 
> outside the class.

OracleVersion.java is gone.  The functionality it provided, access to the fourth element of the version number, is trivially accessed by existing methods in jdk.Version.

jdk.Version is now final, with a private constructor.

An updated webrev is forthcoming.

Regards,
iris

-----Original Message-----
From: Roger Riggs 
Sent: Wednesday, November 25, 2015 1:23 PM
To: core-libs-dev at openjdk.java.net
Subject: Re: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

Hi Iris,

I see the JEP says JDK specific, but does that rule out putting the version API in a Java.* package?
It would support the values found in the java.version, etc properties.
Perhaps as an nested class of System or Runtime?

Version.java:

Line 213:  Seems a bit wasteful to reparse the string after the matcher has done its work;
    but does not use the groups for the version components.

229:  The IAE("Terminal...") exception may not be that easily understood; can it say concisely
    that the build and optional elements are missing

264: current() should cache the value from the parse; it is likely to be called more than once and parsing
     an version string is a relatively expensive.

334: the naming of one of the version elements as 'optional' may be confusing because optional is an adjective.
   Especially when the element is optional and the method named optional returns an Optional.
   Can the name be a cogent noun. how about 'info' for informational string

396:  I'm not sure why BigInteger is needed; other than perhaps because it has a constructor that takes a string.

433:  Optional has a .ifPresent(xxx) method that could be used to streamline the code.
       pre.ifPresent( v -> sb.append("-").append(v));


OracleVersion.java:   Can this be renamed more functionally to reflect 
that the 4'th component is a patch number.
    It might be useful to folks other than Oracle.

107: the constructor should be declared private since it is not needed 
outside the class.

I would have preferred the Tests to be written using TestNG.

Thanks, Roger