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 Jan 13, 2025
1 parent 4efc992 commit 755ab13
Showing 1 changed file with 55 additions and 0 deletions.
55 changes: 55 additions & 0 deletions naga/REVIEW-CHECKLIST.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# 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.

Some of these points are also just coding style issues. Such problems
are probably better to address with comments in the code, or effective
use of Rust (exhaustive match checking, for example).

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

0 comments on commit 755ab13

Please sign in to comment.