[naga] Add a review checklist. #6906
Conversation
jimblandy
force-pushed
the
naga-review-checklist
branch
from
9199174 to
755ab13
Compare
|
This feels like it would be most useful as a GH PR template, where folks could |
naga/REVIEW-CHECKLIST.md
Outdated
| ## General | ||
|
|
||
| If your change iterates over a collection, did you ensure the order of | ||
| iteration was deterministic? Using `HashMap` and `HashSet` is fine, as | ||
| long as you don't iterate over it. | ||
|
|
||
| ## IR changes | ||
|
|
||
| If your change adds or removes `Handle`s from the IR: | ||
| - Did you update handle validation in `valid::handles`? | ||
| - Did you update the compactor in `compact`? | ||
| - Did you update `back::pipeline_constants::adjust_expr`? | ||
|
|
||
| If your change adds a new operation: | ||
| - Did you update the typifier in `proc::typifier`? | ||
| - Did you update the validator in `valid::expression`? | ||
| - If the operation can be used in constant expressions, did you | ||
| update the constant evaluator in `proc::constant_evaluator`? | ||
|
|
||
| ## Backend changes | ||
|
|
||
| - If your change introduces any new identifiers, how did you ensure | ||
| they won't conflict with the users' identifiers? (This is usually | ||
| not relevant to the SPIR-V backend.) | ||
| - Did you use the `Namer` generate a fresh identifier? | ||
| - Did you register the identifier as a reserved word with the the `Namer`? | ||
| - Did you use a reserved prefix registered with the `Namer`? | ||
|
|
||
| ## WGSL Extensions | ||
|
|
||
| If you added a new feature to WGSL that is not covered by of the WebGPU specification: | ||
|
|
||
| - Did you add a `Capability` flag for it? | ||
| - Did you document the feature fully in that flag's doc comment? | ||
| - Did you ensure the validator rejects programs that use the feature? |
There was a problem hiding this comment.
question: I'm concerned about this staying up-to-date, because both contributors and reviewers need to mind this content manually. How will this be achieved?
|
I do wonder how much of this is a culture thing and drilling it in to everyone to rely on the checklist. The one thing I would say is this should be at the root of the repo or in a sub-page of contributing, not in the naga subfolder as it'll too easily get lost. |
As a PR template, wouldn't this be confronting contributors with a long list that is usually almost entirely irrelevant to their PR? I feel like this makes more sense in |
|
I think I'm going to be able to review this on Tuesday of this coming week. 🫡 See you then! |
|
In the maintainers' meeting today, we resolved to move this to the wgpu top-level directory, move the content into a Naga-specific section, and link to it from CONTRIBUTING.md. |
jimblandy
force-pushed
the
naga-review-checklist
branch
4 times, most recently
from
9355503 to
9d4a2eb
Compare
jimblandy
force-pushed
the
naga-review-checklist
branch
3 times, most recently
from
3c9f174 to
a590dad
Compare
jimblandy
force-pushed
the
naga-review-checklist
branch
from
a590dad to
fcbcb3f
Compare
How do folks feel about this? I'm not wedded to the idea, but it seemed like it might be helpful.
https://www.newyorker.com/magazine/2007/12/10/the-checklist