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

Fix inconsistent management of ExtSuffix in INIT macros #453

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

eliemichel
Copy link
Collaborator

@eliemichel eliemichel commented Nov 28, 2024

Proper handling of ExtSuffix was missing from in #422 (could be listed in the followup issue #427).

Note that this does not account for potential namespace, because I am not sure how we should handle it: if a member from a struct foo has type to struct.bar, can we be sure that there is a single bar entry and use its namespace, or should we look for bar first in the same namespace as foo, then fallback to default namespace?

SIde note: neither ExtSuffix not namespaces are tested, we could have a minimal extension example in test.

@eliemichel eliemichel mentioned this pull request Nov 28, 2024
10 tasks
Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

LGTM except of course we'll need to implement #212 (I forgot that actually needed code changes, since it only affected the C header).

I agree it would be really nice to test this. I'll bet something I added in the past few months is handling it wrong. (It would also be a really useful reference for how to add use extension files.)

We can reuse the gen-check pattern but for a sample extension header that we check in under the tests directory. Are you interested in adding it?

Note that this does not account for potential namespace, because I am not sure how we should handle it: if a member from a struct foo has type to struct.bar, can we be sure that there is a single bar entry and use its namespace, or should we look for bar first in the same namespace as foo, then fallback to default namespace?

I'm not sure either. Off the top of my head, either or both:

  • The file format needs to change a bit, because it currently can't distinguish, for example, "add new value to existing enum" vs "add new enum with new value".
  • We should always assume that if something is named in an extension header that exists in the core header, it's referring to the same thing, and error if there are conflicting definitions, e.g. two structs with the same name

@kainino0x kainino0x enabled auto-merge (squash) December 3, 2024 05:08
@kainino0x kainino0x merged commit d22ced3 into webgpu-native:main Dec 3, 2024
5 checks passed
@eliemichel eliemichel deleted the eliemichel/fix-init-ext branch December 5, 2024 20:13
@eliemichel
Copy link
Collaborator Author

We can reuse the gen-check pattern but for a sample extension header that we check in under the tests directory. Are you interested in adding it?

FTR, started in #462!

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.

2 participants