RFR (S): 8022452: Hotspot needs to know about Windows 8.1 and Windows Server 2012 R2

[Coleen Phillimore]

Mikael,

I think this looks great.  It's a lot cleaner (more red than blue changes).

thanks for humoring me,
Coleen

On 08/08/2013 06:25 PM, Mikael Vidstedt wrote:
>
> Coleen,
>
> After having tried numerous different versions I ended up with this:
>
> http://cr.openjdk.java.net/~mikael/webrevs/8022452.3/webrev
>
> Summary of the changes:
>
> * The GetNativeSystemInfo call has been moved up before the switch, 
> but is only called if the os version is recent enough, and  we 
> actually care about the si.wProcessorArchitecture field.
>
> * The true/else branches in the if-diamond have been switch to avoid 
> the negation of the condition.
>
> * While at it I added support for recognizing "Windows Server 2008 R2" 
> (6.1 non-workstation) which we apparently haven't recognized earlier, 
> but which is something the JDK code recognizes [1].
>
> * The " , 64 bit" has been move out below the switch, but is only 
> relevant/tested for on recent enough Windows versions, just like the 
> old code did.
>
> Feedback much appreciated! I'm also investigating if it would be 
> possible to unify/share the code with the java_props_md.c logic [1] to 
> avoid the duplication, but for now this may have to do. In addition to 
> that the code can be cleaned up further if we only support VS2010+, 
> which I believe is de facto since JDK 7 anyway; I will investigate 
> that as a separate improvement.
>
> Thanks,
> Mikael
>
> [1] 
> http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/e057cddf0d6c/src/windows/native/java/lang/java_props_md.c 
> (line 451).
>
> On 2013-08-07 10:56, Coleen Phillimore wrote:
>>
>> Mikael,
>>
>> I do like your new version better but because you changed so much, it 
>> should be verified on all versions of windows.   Which you might not 
>> want to do.   My suggestion to make this code nicer (fix case stmt 
>> and outline common code) was really simple and I think can be 
>> verified by code inspection, which might be less work and easier to 
>> review.
>>
>> Coleen
>>
>> On 8/7/2013 12:28 PM, Mikael Vidstedt wrote:
>>>
>>> Coleen,
>>>
>>> Funny you should mention that, and since I fully agree with you I 
>>> actually did prepare another version of the patch which is heavily 
>>> inspired by the java_props_md.c implementation:
>>>
>>> http://cr.openjdk.java.net/~mikael/webrevs/winvercleanup2/webrev/
>>>
>>> I personally like this version better, but the one thing that held 
>>> me back was the fact that I will basically want to dig up machines 
>>> with all the different (supported) Windows versions and verify that 
>>> the code is still correct in all cases. If you think this is the way 
>>> to go I more than willing to will do so. Let me know what you think 
>>> about the new version of the patch!
>>>
>>> There's no real urgency here - I'd rather do it correct once than...
>>>
>>> Cheers,
>>> Mikael
>>>
>>>
>>> On 2013-08-07 06:39, Coleen Phillimore wrote:
>>>>
>>>> It looks like the code under the case for all these releases, 
>>>> should be different case statements for each, rather than these 
>>>> cascading 'if' statements.  And make 1668-1673 a new function 
>>>> returning si.   So if we have a new release, in general it will 
>>>> work because the default will print out something.   But a better 
>>>> default would be lines 1712-1717 which are dead code now.
>>>>
>>>> Sorry, I know it's simple and urgent but this seems to be getting 
>>>> increasingly ugly.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 8/6/2013 7:36 PM, Mikael Vidstedt wrote:
>>>>>
>>>>> Please review the following change:
>>>>>
>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8022452/webrev
>>>>>
>>>>> The patch adds support to os_windows.cpp for recognizing the 
>>>>> upcoming Windows versions: Windows 8.1 and Windows Server 2012 R2.
>>>>>
>>>>> The changed is based on the corresponding code on the JDK 
>>>>> side[1][2][3].
>>>>>
>>>>> Thanks,
>>>>> Mikael
>>>>>
>>>>> [1] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8020191
>>>>> [2] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/40221b09812f
>>>>> [3] 
>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-July/019463.html
>>>>>
>>>>
>>>
>>
>