[verona.stage] RFR 8087203: Add support for PATCH field and remove unused fields of new version string

Alejandro E Murillo alejandro.murillo at oracle.com
Fri Jun 19 16:53:32 UTC 2015


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



More information about the hotspot-dev mailing list