Skip to content

Remove unused native code and assume unicode is always supported in Win32Natives refresh provider #1415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

HannesWell
Copy link
Member

Unicode support is assumed to be present if the Windows-OS version is greater or equal five, which corresponds to Windows 2000 or later: https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-osversioninfow

For example the JDK binaries provided by Temurin and Oracle require at least Windows-8 for a JDK-17:

and given that even Windows-10 will be EOL in about a year I think we can confidently remove that support for pre Windows 2000 versions.

This was originally part of #1394.

@HannesWell HannesWell changed the title Remove unused native code and assume unicode is always supported Remove unused native code and assume unicode is always supported in Win32Natives refresh provider Jun 10, 2024
@HannesWell HannesWell force-pushed the remove-unused-refresh-native-code branch 2 times, most recently from 12af28b to f4a2bee Compare June 10, 2024 22:11
Copy link
Contributor

github-actions bot commented Jun 10, 2024

Test Results

 1 734 files  ±0   1 734 suites  ±0   1h 22m 48s ⏱️ - 6m 14s
 3 967 tests ±0   3 945 ✅ ±0   22 💤 ±0  0 ❌ ±0 
12 498 runs  ±0  12 337 ✅ ±0  161 💤 ±0  0 ❌ ±0 

Results for commit b86da90. ± Comparison against base commit b9a1842.

♻️ This comment has been updated with latest results.

@jukzi
Copy link
Contributor

jukzi commented Jun 11, 2024

This commit would rely for example on FindFirstChangeNotificationW which is documented to require Windows XP or Windows Server 2003

https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-findfirstchangenotificationw

@HannesWell
Copy link
Member Author

HannesWell commented Jun 11, 2024

This commit would rely for example on FindFirstChangeNotificationW which is documented to require Windows XP or Windows Server 2003

https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-findfirstchangenotificationw

That's correct and the same applies for the removed code, for example FindFirstChangeNotificationA():
https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-findfirstchangenotificationa

Both, the A suffixed and W suffixed methods have been introduced when Unicode support was added in Windows XP:
https://en.wikipedia.org/wiki/Unicode_in_Microsoft_Windows#In_various_Windows_families

Besides that there is only a 64-bit fragment org.eclipse.core.resources.win32.win32.x86_64 and no 32-bit fragment for org.eclipse.core.resources. When looking up the supported architecture I can see support respectively the requirement of 64-bit CPUs only since XP respectively Server 2003:
https://en.wikipedia.org/wiki/List_of_Microsoft_Windows_versions#Personal_computer_versions

With all that org.eclipse.core.resources.win32.win32.x86_64 cannot be used prior Windows XP/Server 2003 anyways and thus the minimally supported version isn't changed by this.

@HannesWell HannesWell force-pushed the remove-unused-refresh-native-code branch 2 times, most recently from 6b8fd28 to b86da90 Compare June 11, 2024 22:03
@jukzi
Copy link
Contributor

jukzi commented Jun 12, 2024

If the "A" and "W" variants where introduced at the same time i don't get if that "A" code was ever used or needed.
Anyway i am OK with removal of "A" variant.

Unicode support was assumed to be present if the Windows-OS version is
greater or equal five, which corresponds to Windows 2000 or later [1].
Because parts of the removed and remaining native code call methods that
are only available on Windows XP/Server 2003 or later, for example
'FindFirstChangeNotificationA()' respectively
'FindFirstChangeNotificationW()', it is save to assume Unicode is always
supported. All methods that check it or handle an alternative encoding
are effectively dead code and can be removed.

Furthermore only 64-bit artifacts are provided now and 64-bit CPU are
not supported before Windows XP, too.

[1] - https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-osversioninfow
@HannesWell HannesWell force-pushed the remove-unused-refresh-native-code branch from b86da90 to bb4c48a Compare June 12, 2024 16:33
@HannesWell
Copy link
Member Author

If the "A" and "W" variants where introduced at the same time i don't get if that "A" code was ever used or needed. Anyway i am OK with removal of "A" variant.

I'm not sure either, so I can only guess.

But then lets not further delay this and submit it.

@HannesWell HannesWell merged commit 25b48e3 into eclipse-platform:master Jun 12, 2024
14 of 15 checks passed
@HannesWell HannesWell deleted the remove-unused-refresh-native-code branch June 12, 2024 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants