[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 |
| ## 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! |
|
|
||
| Ideally, one fixes this kind of problem by refactoring the code so | ||
| that requirements are easier to identify with local reasoning. We do | ||
| this regularly. But when such projects are too large to undertake | ||
| immediately, we can use review checklists as a temporary workaround | ||
| to ensure that all the different spots do get addressed. |
There was a problem hiding this comment.
question: This sounds like an (unwritten) backlog of specific things you had in mind. Do you have issue links for these "projects"? I think that would help a reader gauge the applicability of this paragraph as it ages.
|
|
||
| ## IR changes | ||
|
|
||
| If your change adds or removes `Handle`s from the IR: |
There was a problem hiding this comment.
style: Prettier would add a newline between this paragraph and the list following it. Might be nice for avoiding churn, but no blocking here.
There was a problem hiding this comment.
This also applies to a couple of other places here.
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