KhronosGroup / KhronosGroup/SPIRV-Cross
Review use of assert()?
- Dominant language
- GLSL
- Stars
- 2.5k
- Forks
- 713
- Avg merge
- 2d 18h
- Merged PRs (30d)
- 16
Description
So I ran into this in a Release build of MoltenVK, which catalyzed me to add `NDEBUG` to MoltenVK builds:
https://github.com/KhronosGroup/SPIRV-Cross/blob/d57ab68a219831900fa2b8a3bd529413e01f3b9f/spirv_msl.cpp#L1830-L1831
Unless I am mistaken, this appears to be an assertion that could arise from the content of the SPIR-V, rather than an error in SPIRV-Cross code logic.
Another example is likely here:
https://github.com/KhronosGroup/SPIRV-Cross/blob/d57ab68a219831900fa2b8a3bd529413e01f3b9f/spirv_msl.cpp#L10826
which is probably an indication of a mis-alignment between the incoming SPIR-V content and the level of MSL available on the runtime platform.
This got me wondering whether we should review if some of these `assert()` calls should be converted to exception throws.
The problem with `assert()`, is that it can't be caught be the calling app. I expect that `assert()` should be used only to evaluate code logic in SPIRV-Cross, and exception throws should be used to abort a shader conversion due to any user error, including environmental incompatibilities, or shader content problems, whether it be legal SPIR-V that can't be converted, or illegal SPIR-V that is incorrectly submitted by the app.
A simpler alternative might be to replace all `assert()` calls with a `SPIRV_CROSS_ASSERT()` macro, which will test the assertion condition and then call `SPIRV_CROSS_THROW()`. And if `SPIRV_CROSS_EXCEPTIONS_TO_ASSERTIONS` is defined, this will all revert to an assertions anyway.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by reviewing the assert() calls in spirv_msl.cpp, including the cited locations around lines 1830 and 10826, and compare them with the existing exception behavior. Determine which failures arise from conversion inputs or runtime capabilities versus internal logic, then define a consistent, testable scope for the proposed review.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100