Skip to content
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

platform_types: Convert Windows HANDLE types to isize #797

Merged
merged 1 commit into from
Oct 14, 2023

Conversation

MarijnS95
Copy link
Collaborator

@MarijnS95 MarijnS95 commented Oct 11, 2023

The windows crate treats these as isize rather than raw void pointers:
https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Foundation/struct.HWND.html

And raw-window-handle 0.6 recently started to do the same:
rust-windowing/raw-window-handle#136

However, the win32 documentation still states that these should be PVOID:
https://learn.microsoft.com/en-us/windows/win32/winprog/windows-data-types

Hence I'm not super confident what the right step forward is: isize is a breaking change (we'll do a breaking release soon™™™ anyway) but plays more nicely with interop with other crates. @madsmtm was your motivation for raw-window-handle also the type representation in windows-rs?

@MarijnS95 MarijnS95 force-pushed the platform-types-windows-handle-isize branch from 9b1223a to 95460d4 Compare October 11, 2023 18:35
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ABI-compatible, so I see no harm in conforming to the de facto standard.

The `windows` crate treats these as `isize` rather than raw void
pointers:
https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Foundation/struct.HWND.html

And `raw-window-handle 0.6` recently started to do the same:
rust-windowing/raw-window-handle#136

However, the win32 documentation still states that these should be
`PVOID`:
https://learn.microsoft.com/en-us/windows/win32/winprog/windows-data-types
@MarijnS95 MarijnS95 force-pushed the platform-types-windows-handle-isize branch from ce24c51 to d51682b Compare October 11, 2023 19:13
@madsmtm
Copy link

madsmtm commented Oct 12, 2023

@madsmtm was your motivation for raw-window-handle also the type representation in windows-rs?

Yup, see also discussion in microsoft/windows-rs#1643.

Apologies for not making that change explicitly, I intended to do so but it must have gotten it mixed in with the linked PR.

@MarijnS95
Copy link
Collaborator Author

Thanks @madsmtm, that issue seems rather long and confusing. The TL;DR being that even if a handle may (sometimes?) be a pointer, the user shouldn't be aware of this (and always use appropriate APIs) so an isize to hide that (ignoring the discussion about pointer aliasing rules for the compiler) is appropriate.

I'll go ahead and merge this 👍

@MarijnS95 MarijnS95 merged commit d0d5ea1 into master Oct 14, 2023
22 checks passed
@MarijnS95 MarijnS95 deleted the platform-types-windows-handle-isize branch October 14, 2023 09:18
@MarijnS95
Copy link
Collaborator Author

As noted in the linked windows-rs by me some time ago, these types changed back to raw pointers once again:

For anyone subscribed to this thread, it looks like the types are changed back to pointers recently:

microsoft/win32metadata#1924
microsoft/win32metadata@b4dfd2f

Looks like this on a regen: microsoft/windows-rs@96b9a27

And related discussion rust-windowing/raw-window-handle#171. It doesn't seem like we should blindly change it back though.

@MarijnS95 MarijnS95 added this to the Ash 0.38 milestone Dec 3, 2024
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.

3 participants