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 9355503
Showing 1 changed file with 65 additions and 0 deletions.
65 changes: 65 additions & 0 deletions naga/REVIEW-CHECKLIST.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# Naga Review Checklist

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

Ideally we would keep items off this list entirely:

- Using Rust effectively can turn some mistakes into compile-time
errors. For example, 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, localizing all handle
validation in `Validator::validate_module_handles` allows 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 is a list of concerns that we haven't been able to
address in a more robust way.

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

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

0 comments on commit 9355503

Please sign in to comment.