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

Differentiate NativeTypedef structs that exist in the headers vs. only in metadata #1538

Merged
merged 10 commits into from
Apr 17, 2023

Conversation

mikebattista
Copy link
Contributor

@mikebattista mikebattista commented Apr 14, 2023

Fixed #1533.

If we agree to just introduce a different attribute for metadata-only typedefs, we need to close on the name. I started with NoNativeTypedef. Another option might be MetadataTypedef.

Projections that want to use all the typedefs should just check for both attributes. Projections that want to stay true to the Win32 headers can unwrap MetadataTypedefs.

We could also merge the two attributes like Typedef(Native = true) and Typedef(Native = false).

@riverar

This comment was marked as outdated.

Copy link
Collaborator

@riverar riverar left a comment

Choose a reason for hiding this comment

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

I like the idea of a separate MetadataTypedefAttribute instead. This will allow projection authors to quickly filter out this attribute (if desired). The current implementation requires looking up every single TypedefAttribute to read/compare against its Native value.

@mikebattista
Copy link
Contributor Author

Can you clarify? These MetadataTypedefs would still be applied to functions, so projections need to inspect an attribute and then unwrap values either way whether it's a different attribute or a single attribute with a property.

@riverar
Copy link
Collaborator

riverar commented Apr 17, 2023

The .custom declaraction on, say, HANDLE, after this PR, may look something like:

.custom instance void [Windows.Win32.winmd]Windows.Win32.Foundation.Metadata.TypedefAttribute::.ctor(bool) = (
  01 00 01 00 00
)

A projection author wishing to ignore non-native typedefs will have fully parse the attribute, perhaps something like:

for each attribute in attributes
  if attribute == TypedefAttribute
    if attribute.valueType == bool && getBoolValue(attribute) != true
      continue;
    // ...

The proposed alternative eliminates a bunch of per-attribute work by simply using the type name:

for each attribute in attributes
  if attribute == MetadataTypedef
    continue;
  // ...

@mikebattista
Copy link
Contributor Author

Ok. I'm fine with that to make the processing more efficient. Your earlier comment suggested maybe a different design altogether for the attribute and how it would be applied.

@riverar
Copy link
Collaborator

riverar commented Apr 17, 2023

@mikebattista Oh didn't mean to imply that. Was just agreeing with your "Another option might be MetadataTypedef" 👍

@mikebattista mikebattista merged commit 451f8d6 into main Apr 17, 2023
@mikebattista mikebattista deleted the mikebattista/nativetypedef branch April 17, 2023 21:27
Thomasdezeeuw pushed a commit to rust-lang/socket2 that referenced this pull request Aug 27, 2023
originally Windows SDK didn't have a `sa_family_t` typedef, and we
requested `win32metadata` to add it (microsoft/win32metadata#1538)
and used that from `windows-sys` as our `sa_family_t` definition on
Windows platform (#414), which was an oversight. after all, Winsock2
isn't compliant to Posix and has it's own naming scheme, and
`ADDRESS_FAMILY` is intended to be the equivalence of `sa_family_t`.
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.

Ideally, there would be a distinct attribute for these "invented" handle types
2 participants