Ralith / openxrs

OpenXR bindings for Rust
Apache License 2.0
282 stars 59 forks source link

Fix Windows type alias definitions. #150

Closed jdm closed 9 months ago

jdm commented 9 months ago

https://github.com/Ralith/openxrs/commit/05a0f442d93e627bf45b341100ad09c621d6ab06 replaced the types from the winapi crate with hand-written ones. However, these hand-written ones are incorrect, since they are used as *mut ID3D11Texture2D (for example), which ends up as *mut *mut c_void. This was discovered when updating Servo's webxr crate to use the latest version of openxr.

Ralith commented 9 months ago

The aliases are consistent with their definitions from winapi. Taking your example:

openxrs:

pub type ID3D11Texture2D = *mut c_void;

winapi:

#[repr(C)]
pub struct ID3D11Texture2D {
    pub lpVtbl: *const ID3D11Texture2DVtbl,
}
jdm commented 9 months ago

From the perspective of a user of the API, a *mut *mut c_void is a very different argument to pass than a *mut c_void, particularly when the C SDK expects a ID3D11Texture2D*.

Ralith commented 9 months ago

How does the C SDK define ID3D11Texture2D?

jdm commented 9 months ago

It uses the windows definitions. My argument here is that while functionally it may be equivalent for the rust bindings to accept a pointer to a pointer, it provides very confusing error messages for callers when they try to pass instances of id3d11texture pointers to the rust API.

Ralith commented 9 months ago

It uses the windows definitions

What are those definitions? winapi seems to think they're pointers. Is it wrong?

jdm commented 9 months ago

Ok, sorry, I don't think I've communicated my understanding of the problem well. Winapi defines a struct that matches the windows headers. Openxrs defines it as a pointer, and then generates types with pointers to it. That does not match my understanding of "winapi seems to think they're pointers".

Ralith commented 9 months ago

A C struct containing a pointer has the same representation as a pointer. Still, since we only use these types behind a pointer anyway, I agree that c_void is a clearer choice. Thanks!