Sounds good Joe,
I will push those soon.
Thanks
Alejandro
On 6/19/2015 3:56 PM, huizhe wang wrote:
Hi Alejandro,
Alan's right about the JAXP version-check code. But since this is
urgently needed, I agree you can push the change as is. I'll create a
separate bug to re-examine version related code in jaxp. As Alan
pointed out, it would be some clean-up.
Thanks,
Joe
On 6/19/2015 9:53 AM, Alejandro E Murillo wrote:
Hi Alan, just to you.
didn't hear back from you, so I'll assume you are fine with
the corrections. We want to get this into further testing
so I'm going to push the changes
cheers
Alejandro
On 6/18/2015 4:56 PM, Alejandro E Murillo wrote:
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