Skip to content

Commit

Permalink
[naga] Add a review checklist.
Browse files Browse the repository at this point in the history
  • Loading branch information
jimblandy committed Feb 10, 2025
1 parent 6696c79 commit 3c9f174
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 0 deletions.
3 changes: 3 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,6 @@ you make significant efforts…
The "Assigned" field on a PR indicates who has taken responsibility
for reviewing the PR, not who is responsible for the content of the
PR.

You can see some common things that PR reviewers are going to look for in
[`REVIEW-CHECKLIST.md`].
68 changes: 68 additions & 0 deletions REVIEW-CHECKLIST.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# Review Checklist

This is a collection of notes on things to watch out for when
reviewing pull requests submitted to wgpu and Naga.

Ideally, we want to keep items off this list entirely:

- Using Rust effectively can turn some mistakes into compile-time
errors. For example, in Naga, using exhaustive matching ensures that
changes to the IR will cause compile-time errors in any code that
hasn't been updated.

- Refactoring can gather together all the code responsible for
enforcing some invariant in one place, making it clear whether a
change preserves it or not. For example, Naga localizes all handle
validation to `naga::valid::Validator::validate_module_handles`,
allowing the rest of the validator to assume that all handles are
valid.

- Offering custom abstractions can help contributors avoid
implementing a weaker abstraction by themselves. For example,
because `HandleSet` and `HandleVec` are used throughout Naga,
contributors are less likely to write code that uses a `BitSet` or
`Vec` on handle indices, which would invite bugs by erasing the
handle types.

This checklist gathers up the concerns that we haven't found a
satisfying way to address in a more robust way.

## Naga

### 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.

### WGSL Extensions

- [ ] If you added a new feature to WGSL that is not covered by 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 unless its capability is enabled?

### 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 to generated code,
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` to 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`?

0 comments on commit 3c9f174

Please sign in to comment.