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

Generate QueryInterface-like helper function for callbacks #747

Closed

Conversation

MarijnS95
Copy link
Contributor

Not sure if this is warranted. I longed to use the same neat pattern that landed just today with DXC's DxcCreateInstance function, which is simply a callback on a native function. There's quite a bit of duplication with the implementation in function.rs, too. Curious to hear your thoughts!

@kennykerr
Copy link
Collaborator

Neat, do you know of a Win32 callback that uses this pattern that we could use for a test?

@MarijnS95
Copy link
Contributor Author

Unfortunately I don't, I was testing around with microsoft/win32metadata#450 if that's helpful.

@kennykerr
Copy link
Collaborator

If something's coming down the road, we can wait on this until the metadata is available.

@MarijnS95
Copy link
Contributor Author

@kennykerr I might as well paste what this generates to:

pub type DxcCreateInstance2Proc = unsafe extern "system" fn(
    pmalloc: ::windows::RawPtr,
    rclsid: *const ::windows::Guid,
    riid: *const ::windows::Guid,
    ppv: *mut *mut ::std::ffi::c_void,
) -> ::windows::HRESULT;
pub unsafe fn DxcCreateInstance2Proc<'a, T: ::windows::Interface>(
    func: &DxcCreateInstance2Proc,
    pmalloc: impl ::windows::IntoParam<'a, super::super::Windows::Win32::System::Com::IMalloc>,
    rclsid: *const ::windows::Guid,
) -> ::windows::Result<T> {
    let mut result__ = ::std::option::Option::None;
    (func)(
        pmalloc.into_param().abi(),
        ::std::mem::transmute(rclsid),
        &<T as ::windows::Interface>::IID,
        ::windows::Abi::set_abi(&mut result__),
    )
    .and_some(result__)
}
pub type DxcCreateInstanceProc = unsafe extern "system" fn(
    rclsid: *const ::windows::Guid,
    riid: *const ::windows::Guid,
    ppv: *mut *mut ::std::ffi::c_void,
) -> ::windows::HRESULT;
pub unsafe fn DxcCreateInstanceProc<T: ::windows::Interface>(
    func: &DxcCreateInstanceProc,
    rclsid: *const ::windows::Guid,
) -> ::windows::Result<T> {
    let mut result__ = ::std::option::Option::None;
    (func)(
        ::std::mem::transmute(rclsid),
        &<T as ::windows::Interface>::IID,
        ::windows::Abi::set_abi(&mut result__),
    )
    .and_some(result__)
}

It is allowed to have identical names because the type alias is part of the "type" namespace and the function part of the "value" namespace (https://doc.rust-lang.org/nightly/reference/names/namespaces.html).

@MarijnS95 MarijnS95 marked this pull request as ready for review May 13, 2021 12:41
@MarijnS95
Copy link
Contributor Author

If something's coming down the road, we can wait on this until the metadata is available.

It's here now, see #802 😄

@kennykerr
Copy link
Collaborator

Closing for now as there is no tests to support it. I plan to make it easier to write tests for APIs like this at which point we can revisit this.

@kennykerr kennykerr closed this May 19, 2021
@MarijnS95
Copy link
Contributor Author

You could have given me some time to come up with another test or example after closing #802, instead of closing this outright too.

@MarijnS95
Copy link
Contributor Author

https://docs.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-d3d12createdevice D3D12 has some function pointers that could use this, for example:

pub type PFN_D3D12_CREATE_DEVICE = unsafe extern "system" fn(
    param0: ::windows::RawPtr,
    param1: super::Direct3D11::D3D_FEATURE_LEVEL,
    param2: *const ::windows::Guid,
    param3: *mut *mut ::std::ffi::c_void,
)
    -> ::windows::HRESULT;
pub unsafe fn PFN_D3D12_CREATE_DEVICE<'a, T: ::windows::Interface>(
    func: &PFN_D3D12_CREATE_DEVICE,
    param0: impl ::windows::IntoParam<'a, ::windows::IUnknown>,
    param1: super::Direct3D11::D3D_FEATURE_LEVEL,
) -> ::windows::Result<T> {
    let mut result__ = ::std::option::Option::None;
    (func)(
        param0.into_param().abi(),
        ::std::mem::transmute(param1),
        &<T as ::windows::Interface>::IID,
        ::windows::Abi::set_abi(&mut result__),
    )
    .and_some(result__)
}
pub type PFN_D3D12_CREATE_ROOT_SIGNATURE_DESERIALIZER =
    unsafe extern "system" fn(
        psrcdata: *const ::std::ffi::c_void,
        srcdatasizeinbytes: usize,
        prootsignaturedeserializerinterface: *const ::windows::Guid,
        pprootsignaturedeserializer: *mut *mut ::std::ffi::c_void,
    ) -> ::windows::HRESULT;
pub unsafe fn PFN_D3D12_CREATE_ROOT_SIGNATURE_DESERIALIZER<
    T: ::windows::Interface,
>(
    func: &PFN_D3D12_CREATE_ROOT_SIGNATURE_DESERIALIZER,
    psrcdata: *const ::std::ffi::c_void,
    srcdatasizeinbytes: usize,
) -> ::windows::Result<T> {
    let mut result__ = ::std::option::Option::None;
    (func)(
        ::std::mem::transmute(psrcdata),
        ::std::mem::transmute(srcdatasizeinbytes),
        &<T as ::windows::Interface>::IID,
        ::windows::Abi::set_abi(&mut result__),
    )
    .and_some(result__)
}
pub type PFN_D3D12_CREATE_VERSIONED_ROOT_SIGNATURE_DESERIALIZER =
    unsafe extern "system" fn(
        psrcdata: *const ::std::ffi::c_void,
        srcdatasizeinbytes: usize,
        prootsignaturedeserializerinterface: *const ::windows::Guid,
        pprootsignaturedeserializer: *mut *mut ::std::ffi::c_void,
    ) -> ::windows::HRESULT;
pub unsafe fn PFN_D3D12_CREATE_VERSIONED_ROOT_SIGNATURE_DESERIALIZER<
    T: ::windows::Interface,
>(
    func: &PFN_D3D12_CREATE_VERSIONED_ROOT_SIGNATURE_DESERIALIZER,
    psrcdata: *const ::std::ffi::c_void,
    srcdatasizeinbytes: usize,
) -> ::windows::Result<T> {
    let mut result__ = ::std::option::Option::None;
    (func)(
        ::std::mem::transmute(psrcdata),
        ::std::mem::transmute(srcdatasizeinbytes),
        &<T as ::windows::Interface>::IID,
        ::windows::Abi::set_abi(&mut result__),
    )
    .and_some(result__)
}
pub type PFN_D3D12_GET_DEBUG_INTERFACE =
    unsafe extern "system" fn(
        param0: *const ::windows::Guid,
        param1: *mut *mut ::std::ffi::c_void,
    ) -> ::windows::HRESULT;
pub unsafe fn PFN_D3D12_GET_DEBUG_INTERFACE<T: ::windows::Interface>(
    func: &PFN_D3D12_GET_DEBUG_INTERFACE,
) -> ::windows::Result<T> {
    let mut result__ = ::std::option::Option::None;
    (func)(
        &<T as ::windows::Interface>::IID,
        ::windows::Abi::set_abi(&mut result__),
    )
    .and_some(result__)
}

Too obscure?

@kennykerr
Copy link
Collaborator

We can always reopen PRs if needed. 😊 I closed this PR as its not made any progress in a few weeks and seemed to be dependent on another closed PR. It lacks an issue that it "fixes" - in future please create issues so we can explore problems and solutions before investing time with implementations and code reviews. PFN_D3D12_GET_DEBUG_INTERFACE seems like a reasonable test case - perhaps open an issue to discuss and illustrate how this might be generally helpful. I don't recall how/why this is beneficial for callbacks vs calls.

@MarijnS95
Copy link
Contributor Author

We can always reopen PRs if needed. I closed this PR as its not made any progress in a few weeks and seemed to be dependent on another closed PR.

It took some time to get the rest sorted out, hence remaining stale for a while. For the record, it's always better to discuss how to move forward with the PR from a contributor instead of just closing it and assuming they stopped caring about their contribution too.

It lacks an issue that it "fixes" - in future please create issues so we can explore problems and solutions before investing time with implementations and code reviews.

Unfortunately the hypothetical "Provide an example for DirectXShaderCompiler" issue has already been closed as something not wanted.

There's not really a problem anyway, it's just applying the same neat return-type helper for QueryInterface to callbacks.

PFN_D3D12_GET_DEBUG_INTERFACE seems like a reasonable test case - perhaps open an issue to discuss and illustrate how this might be generally helpful. I don't recall how/why this is beneficial for callbacks vs calls.

I doubt anyone would find that helpful, as the static-linked D3D12GetDebugInterface is available too and probably preferred. Retrieving this function with GetProcAddress and calling it by this PR would be for demonstration purposes only.
If you think demonstrating GetProcAddress, a cast to a callback function signature, and as added bonus this PR to improve the function signature is worth it, I'll reconsider opening an issue about it.

@MarijnS95
Copy link
Contributor Author

Hi @kennykerr, I'm back at this after the recent official deprecation of com-rs, and would like to know if we can respin this?

To re-state my case, with this PR I could write:

let lib = unsafe { Library::new(dxcompiler_lib_name()) }.unwrap();
let create: Symbol<DxcCreateInstanceProc> = unsafe { lib.get(b"DxcCreateInstance\0") }.unwrap();

let compiler: IDxcCompiler2 = unsafe { DxcCreateInstanceProc(&create, &CLSID_DxcCompiler) }?;
let library: IDxcLibrary = unsafe { DxcCreateInstanceProc(&create, &CLSID_DxcLibrary) }?;

Without it, I have to replace the last two lines with this atrocity (using transmute for lack of a non-verbose way to put together all the pointer casts):

let mut compiler = None::<IDxcCompiler2>;
let mut library = None::<IDxcLibrary>;

unsafe { (create.unwrap())(&CLSID_DxcCompiler, &IDxcCompiler2::IID, std::mem::transmute(&mut compiler)).ok()? };
unsafe { (create.unwrap())(&CLSID_DxcLibrary, &IDxcLibrary::IID, std::mem::transmute(&mut library)).ok()? };

let compiler = compiler.unwrap();
let library = library.unwrap();

There are currently 517 hits for pub type PFN_ and there must be some that behave like QueryInterface (i.e. D3D10/11/12 CREATE_DEVICE). If you point out one that you think is understandable for you, I'll see if I can write a test around it while respinning this to adhere to your earlier request. Please reopen in that case and I'll update the branch accordingly.

If there's still a hard feeling for discussing this in a (new) issue rather than PR thread I'll click "reference in new issue" too 😉

@kennykerr
Copy link
Collaborator

Happy to discuss this anywhere, but the value of a new (open) issue is that I'll not likely forget about it. I try to regularly review open issues but there's too much going on for me to remember to check the comments on closed issues. 😜

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