-
Notifications
You must be signed in to change notification settings - Fork 983
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
1 changed file
with
51 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
# Naga Review Checklist | ||
|
||
This is a collection of notes on things to watch out for when | ||
reviewing pull requests submitted to Naga. | ||
|
||
Unfortunately, there are a few parts of Naga where certain knowledge, | ||
requirements, or concerns are spread out across several areas of the | ||
code. This makes mistakes easier, as it is more likely for a | ||
contributor to fix one spot, but neglect others. | ||
|
||
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. | ||
|
||
## 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? |