Handle the rest of ScalarType for wgpu as Undefined#698
Conversation
We are trying to add more types to ScalarType: IntPtr, UIntPtr and BFloat16; in shader-slang/slang#10643 And slang-rhi build prints a warning that the new types are not handled in a switch-statement for WGPU. Because WGPU cannot represent those types, this PR converts the new types as Undefined and avoid the compilation warnings, which is treated an errors on CI machines.
📝 WalkthroughWalkthroughInternal helper functions in the WGPU shader object layout were refactored to consolidate their return statements. The Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wgpu/wgpu-shader-object-layout.cpp (1)
35-37:⚠️ Potential issue | 🟠 MajorSwitch discriminator is wrong; normalized scalar type is ignored.
Line 35 should switch on
scalarType, nottype->getScalarType(). Right now the fallback normalization on Lines 31-34 is bypassed, soNone-typed results are misclassified (e.g., forced toFloatat Line 37).Suggested fix
- switch (type->getScalarType()) + switch (scalarType) { case slang::TypeReflection::ScalarType::None: return WGPUTextureSampleType_Float; case slang::TypeReflection::ScalarType::Void: case slang::TypeReflection::ScalarType::Bool: return WGPUTextureSampleType_Undefined; @@ + case slang::TypeReflection::ScalarType::IntPtr: + case slang::TypeReflection::ScalarType::UIntPtr: + case slang::TypeReflection::ScalarType::BFloat16: + return WGPUTextureSampleType_Undefined; default: break; }Also applies to: 56-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wgpu/wgpu-shader-object-layout.cpp` around lines 35 - 37, The switch is using type->getScalarType() directly and thus ignores the normalized variable scalarType; change the switch to use the normalized scalarType (the local scalarType variable computed just above) so the fallback normalization is respected in cases like None; update both switch sites (the one handling the first scalar mapping and the second occurrence around the later mapping) to switch on scalarType instead of calling type->getScalarType() so values like None are classified correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/wgpu/wgpu-shader-object-layout.cpp`:
- Around line 35-37: The switch is using type->getScalarType() directly and thus
ignores the normalized variable scalarType; change the switch to use the
normalized scalarType (the local scalarType variable computed just above) so the
fallback normalization is respected in cases like None; update both switch sites
(the one handling the first scalar mapping and the second occurrence around the
later mapping) to switch on scalarType instead of calling type->getScalarType()
so values like None are classified correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 271c5363-d6c0-4b2a-bbe2-52c3dde7d842
📒 Files selected for processing (1)
src/wgpu/wgpu-shader-object-layout.cpp
We are trying to add more types to ScalarType: IntPtr, UIntPtr and
BFloat16; in shader-slang/slang#10643
And slang-rhi build prints a warning that the new types are not handled
in a switch-statement for WGPU.
Because WGPU cannot represent those types, this PR converts the new
types as Undefined and avoid the compilation warnings, which is treated
an errors on CI machines.