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

Code Quality: Switch Storage Provider Status UI APIs to use MemberFunction #17034

Merged
merged 1 commit into from
Apr 11, 2025

Conversation

dongle-the-gadget
Copy link
Contributor

Resolved / Related Issues

Switch the Storage Provider Status UI APIs from the Stdcall calling convention to MemberFunction, for correctness.

Steps used to test these changes

Tested the Properties window of OneDrive, works as expected.

@0x5bfa
Copy link
Member

0x5bfa commented Apr 10, 2025

Since CsWin32 uses StdCall for COM functions, I thought okay with this convention. It uses a wrong conversion?

@dongle-the-gadget
Copy link
Contributor Author

MemberFunction is more strict, and for our purposes here it differentiates between C# int and C++ HRESULT (which is actually a type alias for int) versus CsWin32 HRESULT type, which is a wrapper around int.
The fact that Stdcall lets us use CsWin32 HRESULT instead of int is kind of by accident, and MemberFunction disallows that.

@0x5bfa
Copy link
Member

0x5bfa commented Apr 10, 2025

Is that because the layout the HRESULT from CsWin32 matches the one of int (2 bytes)?

@dongle-the-gadget
Copy link
Contributor Author

Quote from Jeremy (.NET):

MemberFunction “doesn’t work” with an HResult struct as that’s technically wrong. That’s the whole reason we needed to add it (as an HResult struct works with Stdcall)

As for why it worked, it has to do with registers (that I’m not qualified to talk about)

Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

Thanks.

@yaira2 yaira2 merged commit ae7432d into files-community:main Apr 11, 2025
6 checks passed
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Apr 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants