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: Replaced stack-allocated GUIDs with RVA-inlined GUIDs #16993

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Mar 29, 2025

Resolved / Related Issues

FWIW, I also categoried NativeMethods.txt into Functions, Structs, COM, Constants and Enums in order to exactly track what to generate.

Steps used to test these changes

  1. Build Files
  2. Open Files

CC: @dongle-the-gadget I used your SG to do this; I'd like to request your review.

Copy link
Contributor

@dongle-the-gadget dongle-the-gadget left a comment

Choose a reason for hiding this comment

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

  1. Is the removal of ThrowOnFailure intended?
  2. Can you use the generator on the Storage Provider IID property as well?

@0x5bfa
Copy link
Member Author

0x5bfa commented Mar 31, 2025

  1. I wasnt sure if CoCreateInstance fails in any way, these COM interfaces should exist, right?
  2. Done.

@dongle-the-gadget
Copy link
Contributor

  1. CoCreateInstance could possibly fail, for instance Storage Provider Status would in Windows 10.
  2. It seems like the Guid properties in IStorageProviderStatusUI and co. are still manually defined?

@0x5bfa
Copy link
Member Author

0x5bfa commented Mar 31, 2025

  1. Coclasses of IFileOpenDialog and IDesktopWallpaper should persist across Windows 10 and 11 versions. But still Im finding a better way to handle HResults in the app (maybe Im gonna return true/false idk) since exception impacts performance iirc.
  2. var guid = *IID.IID_IID_IStorageProviderStatusUISourceFactory is fine here?

@dongle-the-gadget
Copy link
Contributor

2 is probably okay, but personally I would use GuidRVAGen on the ref readonly property and then use Unsafe and MemoryMarshal on the pointer one.

@0x5bfa 0x5bfa force-pushed the 5bfa/CQ-WindowsStorable2 branch from 4851a8e to 41ca094 Compare April 1, 2025 16:33
@0x5bfa
Copy link
Member Author

0x5bfa commented Apr 1, 2025

Now this is alright?

@0x5bfa
Copy link
Member Author

0x5bfa commented Apr 4, 2025

@yaira2 This is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants