From 755ab13eae2ec24d1b069b066a103fc94bbc44cb Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Mon, 13 Jan 2025 12:01:34 -0800 Subject: [PATCH] [naga] Add a review checklist. --- naga/REVIEW-CHECKLIST.md | 55 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 naga/REVIEW-CHECKLIST.md diff --git a/naga/REVIEW-CHECKLIST.md b/naga/REVIEW-CHECKLIST.md new file mode 100644 index 0000000000..1a8c5d3f4d --- /dev/null +++ b/naga/REVIEW-CHECKLIST.md @@ -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?