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 generation of docs on array members #447

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

kainino0x
Copy link
Collaborator

Fixes #431

@kainino0x kainino0x requested a review from eliemichel November 28, 2024 00:55
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.

Looks all right, only a minor note about comment formatting!

gen/cheader.tmpl Outdated
@@ -339,8 +339,15 @@ typedef struct WGPU{{.Name | PascalCase}}{{$.ExtSuffix}} {
WGPUChainedStructOut chain;
{{- end}}
{{- range $memberIndex, $_ := .Members}}
{{- if (.Type | IsArray)}}
/** Array count for `{{.Name | CamelCase}}`. The `INIT` macro sets this to 0. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we use MComment here rather, or at least format the comment on multiple lines like all other docstrings? Saying this for the sake of consistency, though on the other hand we know by construction that this comment will be single line while other member comment blocks probably won't!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I prefer it this way because it's more straightforward. MComment and friends seem to be mostly needed because they can deal with multiline inputs (that usually come from the yaml file) whereas in this case I think it's more straightforward to just do the templating inline, otherwise I think I'd need to either add a new MCommentArrayCount, or otherwise have it be a bit messy like:

    {{- MComment (print "Array count for " (.Name | CamelCase) ". The `INIT` macro sets this to 0.") 4 }}

which doesn't seem that helpful (though TIL I can use print which makes it possible at all). I do have conflicting feelings about whether I should use a 1-line doc comment or 3-line /** * */. I used 1-line because the useful documentation is on the other field, OTOH it feels very out of place. I think I will change it to 3-line.

MCommentArrayCount is also a bit of a special case, because the pattern of splitting arrays into count+pointer is somewhat C-target-specific, though I'm sure there are some other targets that would use it, like C++ without std::span.

I have very much been doing Whatever Works in this code generator, because I am new to Go and Go templating. @rajveermalviya is free to feel that I have made a mess in their code generator; I'm certain I could be doing things better. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Landing now but happy to follow up on anything.

@kainino0x kainino0x merged commit b787f0f into webgpu-native:main Dec 3, 2024
5 checks passed
@kainino0x kainino0x deleted the array-docs branch December 3, 2024 04:28
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.

Fix documentation on size/pointer pairs for array members
2 participants