Closed MarcelStruckWO closed 3 months ago
🐞Mistake | 🤪Typo | 🚨Security | 🚀Performance | 💪Best Practices | 📖Readability | ❓Others |
---|---|---|---|---|---|---|
0 | 0 | 0 | 0 | 0 | 0 | 0 |
wgsl_bindgen
version from 0.14.1 to 0.15.0 in example/src/shader_bindings.rs
.SourceHash
in example/src/shader_bindings.rs
.cache: None
to the ComputeModuleBuilder
in wgsl_bindgen/src/generate/shader_module.rs
.cache: None
to the test cases in wgsl_bindgen/src/generate/shader_module.rs
.cache: None
to the expected output in wgsl_bindgen/tests/output/bindgen_main.expected.rs
.cache: None
to the expected output in wgsl_bindgen/tests/output/bindgen_minimal.expected.rs
.wgsl_bindgen/tests/output/bindgen_minimal.expected.rs
and wgsl_bindgen/tests/output/bindgen_padding.expected.rs
for better readability.ID | Type | Details | Severity | Confidence |
---|---|---|---|---|
1 | 💪Best Practices | The cache property is set to None without any comments or documentation explaining why. This might lead to confusion for future maintainers. |
🟠Medium | 🟠Medium |
cache
property is set to None
without any comments or documentation explaining why.In the following lines of code, the cache
property is set to None
without any explanation or documentation:
wgsl_bindgen/src/generate/shader_module.rs
lines 206, 661, 676wgsl_bindgen/tests/output/bindgen_main.expected.rs
lines 324, 345wgsl_bindgen/tests/output/bindgen_minimal.expected.rs
lines 224, 229wgsl_bindgen/tests/output/bindgen_padding.expected.rs
lines 224, 229This might lead to confusion for future maintainers who might not understand the reasoning behind this decision.
impl<'a> ComputeModuleBuilder<'a> {
pub fn build(self) -> Result<ComputeModule, wgpu::PipelineCreationError> {
Ok(ComputeModule {
module: &module,
entry_point: #entry_point,
compilation_options: Default::default(),
// Cache is set to None to avoid potential issues with stale cache data.
// This might be revisited in future versions.
cache: None,
})
}
}
The fix adds a comment explaining why the cache
property is set to None
. This helps future maintainers understand the reasoning behind this decision and can revisit it if needed.
The current changes do not introduce any new functionality that requires additional tests. The changes are primarily related to setting the cache
property to None
and updating the version and source hash. Existing tests should cover the functionality adequately.
Summon me to re-review when updated! Yours, Gooroo.dev Your opinion matters! React or reply to share it.
Thank you.
Fix #41