Skip to content

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

Merged
mikebattista merged 10 commits intomainfrom
mikebattista/nativetypedef
Apr 17, 2023
Merged

Differentiate NativeTypedef structs that exist in the headers vs. only in metadata#1538
mikebattista merged 10 commits intomainfrom
mikebattista/nativetypedef

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).

@mikebattista mikebattista force-pushed the mikebattista/nativetypedef branch from 715e1c9 to 2579722 Compare April 14, 2023 16:28
@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