Add Windows Version Helper functions #1050

dbwiddis · 2019-01-06T08:12:22Z

See reference docs. Code directly adapted from header file here.

matthiasblaesing

I left a few comments inline. Thank you.

matthiasblaesing · 2019-01-07T19:16:52Z

contrib/platform/src/com/sun/jna/platform/win32/Kernel32.java

+     *         If the function fails, the return value is zero and GetLastError
+     *         returns an error code other than ERROR_OLD_WIN_VERSION.
+     */
+    boolean VerifyVersionInfoW(OSVERSIONINFOEX lpVersionInformation, int dwTypeMask, long dwlConditionMask);


This should be VerifyVersionInfo -- but that would not work, because OSVERSIONINFOEX is only correctly bound for unicode?! Crap...

I tried VerifyVersionInfo but got UnsatisfiedLinkError: Error looking up function 'VerifyVersionInfo'. Both the W and A variants worked individually in this case because we're only using the numeric portions of OSVERSIONINFOEX. The inline functions in the header file use the W version, so that's the one I implemented. Should I add the A version as well?

Er, now I see what you mean. This method should properly use OSVERSIONINFOEXW which is not defined (but the OSVERSIONINFOEX defined is the W version.)

I had to go look at git blame to make sure I wasn't at fault there.

So I'm thinking about the correct way to handle this. One thought was to copy the OSVERSIONINFOEX structure to OSVERSIONINFOEXW (have one extend the other) and mark the non-W version as deprecated. But there are many other functions which also call it, so we'd have to change all of them as well. Even though per the Windows documentation, all these old functions are also deprecated in favor of the Version Helper functions, which currently still refer to this structure... and around in circles we go.

I do not see a good option to fix this without breaking API, so at this point in time, I think binding it as VerifyVersionInfoW is a good solution.

matthiasblaesing · 2019-01-07T19:20:03Z

contrib/platform/src/com/sun/jna/platform/win32/Kernel32.java

+     *            dwTypeMask parameter indicates the members of this structure
+     *            that contain information to compare.
+     * 
+     *            You must set the dwOSVersionInfoSize member of this structure


you need to add p-tags to get the paragraph formatted (javadoc is HTML). The newlines are removed when rendered to HTML.

matthiasblaesing · 2019-01-07T19:22:23Z

contrib/platform/src/com/sun/jna/platform/win32/Kernel32.java

+     * behavior.
+     * 
+     * @param lpVersionInformation
+     *            A pointer to an OSVERSIONINFOEX structure containing the


it would be great if embedded code/code like sequences would be surrounded by {@code } and if possible @link to the class. That is not required, but people might like it :-)

matthiasblaesing · 2019-01-07T19:25:53Z

contrib/platform/src/com/sun/jna/platform/win32/WinNT.java

+    byte VER_LESS = 4;
+    byte VER_LESS_EQUAL = 5;
+    byte VER_AND = 6;
+    byte VER_OR = 7;


Is this a source/binary compatible change (will code compiled against 5.2 still work with 5.3 (or vice-versa)? I see why you do it, but if if is not compatible, please stay with int.

I searched the JNA codebase and don't see anywhere else where these constants are referenced.

External code using JNA 5.2 or earlier may use these as int, but a byte will work anywhere an int is expected with a widening conversion transparent to the user. The alternative is to require users to explicitly cast these all to byte to use them with VerSetConditionMask (and potentially other functions).

Given that byte-to-int happens transparently to the user, but int-to-byte requires an explicit cast, I do not think this breaks compatibility and byte is preferred here.

Well apparently it's not backwards compatible, which surprises me! But I'll put it back to int and then cast.

matthiasblaesing · 2019-01-07T19:40:28Z

contrib/platform/src/com/sun/jna/platform/win32/VersionHelpers.java

+     * @return true if the current OS version matches, or is greater than, the
+     *         Windows XP version.
+     */
+    public static boolean IsWindowsXPOrGreater() {


I'm not sure whether I like this, but I don't see a saner version, that doesn't look like repeats...

What might be worth evaluation is, if pulling HIBYTE(WinNT.WIN32_WINNT_WINXP) and LOBYTE(WinNT.WIN32_WINNT_WINXP) into private static final variables might be worth it.

I did try to faithfully reproduce the existing inline header file, in the interest of readability.

I think adding variables would be worse than just explicitly doing the math ((byte) (word >>>8)) and (byte) word inline, which would be pre-calculated and optimized by the compiler. (I expect the JIT optimizer would also eventually optimize the method calls as well if they were called frequently enough.)

Sorry! I did not realize, that I had looked at an older SDK, that did not contain the header. And I just realized, that you linked the header. Your first version was better readable.

So you want me to go back to the HIBYTE and LOBYTE methods (that would be JIT optimized) rather than the current compiler-optimized bit math? I kind of like the current version and think adding a comment would make it just as readable/understandable for future maintainers.

My thinking was, that at this time I understand what you are doing there. But in 6-12 months I might have a WTF moment. I'm ok if you want to keep the bit shifts inline.

Independend of this - my original idea was this:

private static final byte WIN32_WINNT_WINXP_MAJOR = HIBYTE(WinNT.WIN32_WINNT_WINXP); private static final byte WIN32_WINNT_WINXP_MINOR = LOBYTE(WinNT.WIN32_WINNT_WINXP); /** * @return true if the current OS version matches, or is greater than, the * Windows XP version. */ public static boolean IsWindowsXPOrGreater() { return IsWindowsVersionOrGreater(WIN32_WINNT_WINXP_MAJOR, WIN32_WINNT_WINXP_MINOR, 0); }

Pulling the calculation into a static final. But as said, if you like it this way, I won't stand in the way.

I'll add a comment explaining what the bit shifts do, so you and I both can remember what's going on in 6-12 months.

matthiasblaesing · 2019-01-07T19:41:10Z

CHANGES.md

@@ -7,6 +7,7 @@ Next release (5.2.1)

 Features
 --------
+* [#1050](https://github.com/java-native-access/jna/pull/1050): Add `c.s.j.p.win32.VersionHelpers` and supporting functions - [@dbwiddis](https://github.com/dbwiddis).


Please update the version behind "Next release" to "5.3.0", as it will be a feature release with this change.

Thought so and somewhat expected this comment. :)

matthiasblaesing · 2019-01-09T19:58:19Z

contrib/platform/test/com/sun/jna/platform/win32/VersionHelpersTest.java

@@ -0,0 +1,66 @@
+package com.sun.jna.platform.win32;


I missed this in the first round - could you please also the ALv2+LGPL header here? I'd like to converge on that.

Sure, will do! I just copied some other random test file that was missing a header so didn't know if that was standard.

matthiasblaesing · 2019-01-10T18:53:45Z

Thank you - merging.

Add Windows Version Helper functions

54d9c51

dbwiddis force-pushed the version-helper branch from 9849f56 to 54d9c51 Compare January 6, 2019 08:16

matthiasblaesing reviewed Jan 7, 2019

View reviewed changes

Javadocs, better typecasting

3bba92c

dbwiddis mentioned this pull request Jan 8, 2019

WinXP Compatibility for PdhUtil#PdhLookupPerfNameByIndex #1052

Merged

matthiasblaesing reviewed Jan 9, 2019

View reviewed changes

Comments and license header

5611ae6

matthiasblaesing merged commit 9a1be16 into java-native-access:master Jan 10, 2019

dbwiddis deleted the version-helper branch February 4, 2019 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Windows Version Helper functions #1050

Add Windows Version Helper functions #1050

dbwiddis commented Jan 6, 2019

matthiasblaesing left a comment

matthiasblaesing Jan 7, 2019

dbwiddis Jan 7, 2019 •

edited

Loading

dbwiddis Jan 7, 2019

dbwiddis Jan 8, 2019

matthiasblaesing Jan 8, 2019

matthiasblaesing Jan 7, 2019

matthiasblaesing Jan 7, 2019

matthiasblaesing Jan 7, 2019

dbwiddis Jan 7, 2019

dbwiddis Jan 8, 2019

matthiasblaesing Jan 7, 2019

dbwiddis Jan 7, 2019 •

edited

Loading

matthiasblaesing Jan 8, 2019

dbwiddis Jan 8, 2019

matthiasblaesing Jan 9, 2019

dbwiddis Jan 9, 2019

matthiasblaesing Jan 7, 2019

dbwiddis Jan 7, 2019

matthiasblaesing Jan 9, 2019

dbwiddis Jan 9, 2019

matthiasblaesing commented Jan 10, 2019

Add Windows Version Helper functions #1050

Add Windows Version Helper functions #1050

Conversation

dbwiddis commented Jan 6, 2019

matthiasblaesing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasblaesing commented Jan 10, 2019

dbwiddis Jan 7, 2019 •

edited

Loading

dbwiddis Jan 7, 2019 •

edited

Loading