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

[Alejandro E Murillo]

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