Skip to content

Latest commit

 

History

History
68 lines (52 loc) · 2.7 KB

REVIEW-CHECKLIST.md

File metadata and controls

68 lines (52 loc) · 2.7 KB

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