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

Some experiments with HString wrapper and ComPtr #3

Closed
wants to merge 40 commits into from

Conversation

Boddlnagg
Copy link
Collaborator

@Boddlnagg Boddlnagg commented May 16, 2016

This is not meant to be merged!

It shows my progress so far and allows me to share it with you more easily. Note that it requires a local override of winapi-rs with the necessary changes (either my or your PR).

What is currently definitely wrong in my implementation is:

  • The copy of the RIDL! macro from winapi. Either mark it as export in winapi or build a custom one with more features (I thought about having a way to automatically assign IIDs to interfaces, in order to get rid of the duplication in lines such as asyncOp.query_interface::<IUnknown>(&IID_IUnknown); and be able to write asyncOp.query_interface::<IUnknown>(); instead (UPDATE: I've added a commit which does extend the macro accordingly. We have to see whether we are able to upstream this to winapi-rs.)
  • The implementation of the completion handler. I built a custom COM class with manual reference counting that pretends to be of the right type, but this is probably totally wrong. You said you had already got completion handlers working, so you can just ignore that part of my code. (UPDATE: It seems to be not that wrong actually, so I refactored/simplified it a bit. The only remaining question there is: Which interfaces does the handler support in QueryInterface? My test code reports which interfaces are queried.)

(By the way, I probably won't have much time to work on this during the next weeks.)

@contextfree
Copy link
Owner

Thanks! I probably won't get a chance to seriously look/work on this until Tuesday or Wednesday.

@Boddlnagg
Copy link
Collaborator Author

Oh, one more comment: Another reason that we probably can't rely on winapi's RIDL macro, is that we sometimes need generic definitions, e.g. IAsyncOperation<T>.

@Boddlnagg
Copy link
Collaborator Author

Boddlnagg commented Jun 1, 2016

This might also be helpful: retep998/winapi-rs#145 (comment). It's about how to implement a COM interface from Rust.

@Boddlnagg Boddlnagg force-pushed the experimental branch 2 times, most recently from 487ea5f to 1000edb Compare June 3, 2016 19:24
Also replaced tabs with spaces.
@Boddlnagg
Copy link
Collaborator Author

Also please note that my test code still requires a change in winapi: Add #[derive(PartialEq, Eq)] to GUID in guiddef.rs.

@Boddlnagg
Copy link
Collaborator Author

... this last commit renders my previous comment obsolete.

@Boddlnagg
Copy link
Collaborator Author

Boddlnagg commented Jun 6, 2016

This is now in a state where it actually starts looking like more than a proof-of-concept. I'd like to have some feedback and then we should get started with the automatic code-generation.

I wonder if we can get rid of this out-pointer-passing in the low-level-bindings already? I think we should generate the impl for the function definitions (which forward to the vtbl) already in such a way that they

  1. ... take care of creating the unitialized out-pointers for the return values, check the HRESULT and return Result<T, HRESULT> (or some custom error type)
  2. ... already have a snake_case name. (As far as I understand it, language projections should map to the respective naming conventions.)

This is not possible with macros, but should easy with custom codegen, as long as we're careful with possible null return values.

@Boddlnagg Boddlnagg force-pushed the experimental branch 2 times, most recently from 84e08dc to 70bc235 Compare June 8, 2016 16:50
@Boddlnagg Boddlnagg closed this Jul 15, 2016
@Boddlnagg Boddlnagg mentioned this pull request Jul 15, 2016
@Boddlnagg Boddlnagg deleted the experimental branch July 21, 2016 20:29
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