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

Add windows-result crate #2847

Merged
merged 9 commits into from
Feb 15, 2024
Merged

Add windows-result crate #2847

merged 9 commits into from
Feb 15, 2024

Conversation

kennykerr
Copy link
Collaborator

This update pulls out the error handling implementation from the windows-core crate and making it available directly without a dependency on the windows or windows-core crates. Windows error handling can be tricky. This should make it simpler for library and component authors to use and provide Windows error handling and propagation in more scenarios. Notably:

  • These types are still included in the windows-core and windows crates as a dependency so it should not affect existing code too much.

  • This depends on the low-level vtable generation provided by Add "vtbl" option for low-level interface vtable generation #2845 and provides a simple example of how one might go about using that.

  • The Error and HRESULT types have been simplified a little. Mainly they no longer force you to convert to and from HSTRING types. You can use a str to create a new Error value and you can retrieve the error messages from an HRESULT or Error value directly as a String.

@kennykerr
Copy link
Collaborator Author

@ridwanabdillahi I moved some types from windows-core to windows-result - does the new crate needs its own natvis? Haven't had much success debugging the test failure.

@tim-weis
Copy link
Contributor

The respective parts of the .natvis implementation need to move and have their <Type> elements updated appropriately. The .natvis also needs to be wired up, which currently happens here:

#![cfg_attr(windows_debugger_visualizer, debugger_visualizer(natvis_file = "../windows.natvis"))]

The debugger_visualizer tests need to be updated as well. (I can't prepare a PR right now being on a rather useless internet connection unfortunately.)

@ridwanabdillahi
Copy link
Contributor

Sorry for the delay but Tim is correct. Since the crate housing certain types have changed, the Natvis definitions for those types need to be updated to specify the correct fully qualified name of these types. For instance windows_core::error::Error will need to change to windows_result::error::Error.

@kennykerr
Copy link
Collaborator Author

Thanks for the quick fix @riverar!

@tim-weis
Copy link
Contributor

The .natvis file still needs to be split in two and associated with each crate. The test currently succeeds by coincidence. A test that only references windows-result would indeed fail, as no debugger visualizers are compiled into the windows-result PDB.

While at it I would also suggest renaming the .natvis file following the crate name (i.e., windows-core.natvis and windows-result.natvis). This makes it a lot easier to spot when multiple .natvis files are compiled into a PDB using the .nvlist debugger command.

@kennykerr
Copy link
Collaborator Author

Thanks Tim, yes it makes sense to split them up and have per-crate natvis files. I'd also like to greatly simplify the testing as its currently too difficult to figure out what's wrong and how to fix it. Anyway, I'll leave that for another PR. Feel free to jump in to help if you would like.

@kennykerr kennykerr merged commit d52ecc5 into master Feb 15, 2024
65 checks passed
@kennykerr kennykerr deleted the result branch February 15, 2024 19:57
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.

4 participants