Thanks Alan,
see below
On 6/18/2015 7:41 AM, Alan Bateman wrote:
On 16/06/2015 23:55, Alejandro E Murillo wrote:
Please review these changes:
Bug: https://bugs.openjdk.java.net/browse/JDK-8087202
Webrev: http://cr.openjdk.java.net/~amurillo/9/8087202
The implementation of isJavaVersionAtLeast in the JAXP classes look
okay although I think this is code that could be removed. Joe Wang can
confirm but I think it dates back to when the JAXP API was a
standalone API and there was an attempt to keep the code in sync
across major versions. I'll create a separate bug re-examine this as
it looks like some clean-up can be done here.
sounds good,
in general, that code can be called a lot, so changes need to be
carefully done
as to avoid perf impact. So it might be better if that check can be removed
Just on the comment "In JDK9 the version string was changed ..." will
date quickly and would be nice to say that "In JDK 8 and older then it
assumes 1.N and for JDK 9 and newer it assumes N.
In sun.misc.Version.initVersions then InternalError instead of
RuntimeException might be more appropriate as something is really
broken if that happens.
Indeed. Changed.
Also as David pointed out, it shouldn't be @since JDK9 in the new
method. This reminds me to check if the JEP says anything about @since
tags because we already have quite a few @since 1.9.
yeah, I had meant to double check that. Will change it to 9
In the Version.java test then the line with the pattern is really
long, maybe that could be split up to make future side-by-side views
easier to read.
sure, will make it into several lines, based on components, like this:
String jep223Pattern =
"^([0-9]+)(\\.([0-9]+))?(\\.([0-9]+))?(\\.([0-9]+))?" + // $VNUM
"(-([a-zA-Z]+))?(\\.([a-zA-Z]+))?" + // $PRE
"(\\+([0-9]+))?" + // Build Number
"(([-a-zA-Z0-9.]+))?$"; // $OPT
see new webrev:
http://cr.openjdk.java.net/~amurillo/9/8087202.v2/jdk/src/java.base/share/classes/sun/misc/Version.java.template.udiff.html
Otherwise looks okay to me.
great, thanks!
Here's the new webrev (I tested these with JPRT, testsets hotspot and pit):
http://cr.openjdk.java.net/~amurillo/9/8087202.v2/
Thanks!
--
Alejandro
