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

Proposal: enums in INIT macros always use Undefined #446

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

kainino0x
Copy link
Collaborator

The actual default may be dependent on where the struct is used, or could be changed from a trivial default to a non-trivial default in the future.

The Undefined enum values (or WGPUCompositeAlphaMode_Auto) are defined to always do the defaulting inside the API regardless of whether it's a trivial default or not.

All enum fields (searched for type: enum\..*\n\s*default: ) now use Undefined(/Auto) or *NotUsed. (except for #445)

I also added documentation to all the ones I changed, explaining the default. All of the others are either optional or required in the JS spec, so should be fairly clear.

I might turn the defaulting/optional/required docs into something more generated later, but for now this does what we need.

@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Nov 27, 2024
@kainino0x kainino0x requested a review from eliemichel November 27, 2024 04:21
@kainino0x kainino0x mentioned this pull request Nov 27, 2024
10 tasks
Copy link
Collaborator

@eliemichel eliemichel left a comment

Choose a reason for hiding this comment

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

If I understand correctly, setting e.g. WGPUBlendComponent to Undefined will have the same effect as setting it to Add (in the current stats of the API), so it is preferred to init it to Undefined.

I see the point of automatically handling future updates of the API, but on the other hand it makes the header much less self-contained (and knowing the default values is one of the main reasons why I always have to look up the docs).

I'm tempted to suggest we adopt a soft trade-off where the default value is in effect Undefined but the docstring explicitly tells what non-undefined value this corresponds to. We're back with the risk of doc being out of sync with the actual behavior but at least it solves the issue of expressing non-trivial defaulting scheme.

We could phrase the doc such that we invite one to check out a reliable source of info about where to find up-to-date information about the defaulting mechanism.

@kainino0x
Copy link
Collaborator Author

@eliemichel Sorry - I completely forgot to push my second commit before opening this PR (which I was referring to in my PR summary). It adds documentation for all of the defaulting. WDYT?

@eliemichel eliemichel self-requested a review November 27, 2024 23:46
Copy link
Collaborator

@eliemichel eliemichel left a comment

Choose a reason for hiding this comment

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

Solves my concern indeed!

@kainino0x kainino0x merged commit 9e8d097 into webgpu-native:main Nov 28, 2024
5 checks passed
@kainino0x kainino0x deleted the init-undefined branch November 28, 2024 01:22
@kainino0x
Copy link
Collaborator Author

Landing tentatively - pretty sure this is what we'll settle on, but we'll !discuss to verify.

@kainino0x
Copy link
Collaborator Author

Dec 9 meeting:

  • KN: (summary)
  • CF: Don't love it but seems fine. Prefer not to lose documentation
  • KN: Agree, I added documentation for all of these in the PR.

@kainino0x kainino0x removed the !discuss Needs discussion (at meeting or online) label Dec 9, 2024
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