[glsl-in] fix missing side effects from sequence expressions #8787
Conversation
Vipitis
changed the title
| @@ -0,0 +1,19 @@ | |||
| // issue #6208 https://github.com/gfx-rs/wgpu/issues/6208 | |||
| # version 460 | |||
There was a problem hiding this comment.
would it be preferred to adopt webgl cts tests, for example https://registry.khronos.org/webgl/sdk/tests/conformance/glsl/bugs/sequence-operator-evaluation-order.html?
Vipitis
changed the title
There was a problem hiding this comment.
Thanks for this PR, its very useful I believe! One little nit in the comments plus one question:
Whats with all of these weirdly formatted comments? Are they taken from a spec somewhere? Just meant to guide you? I don't know if we should keep them or not
| for &e in &exprs[1..] { | ||
| meta.subsume(stmt.hir_exprs[e].meta); | ||
| } |
There was a problem hiding this comment.
Thanks for handling the spans correctly
| return Err(Error { | ||
| kind: ErrorKind::SemanticError("Empty expression sequence".into()), | ||
| meta, | ||
| }) |
There was a problem hiding this comment.
If this can't be produced by the parser its more clear to say unreachable!()
There was a problem hiding this comment.
Lots of non-functional changes here, see overall review comment
|
I will address the review later (1-2 days hopefully).
It's from the GLSL spec, chapter 9 shader grammar. to help me read the code initially. As the functions don't exactly match the productions and tokens there I added in the places I looked at to better understand what's happening. Sadly the grammar doesn't appear to be any kind of normalized CFG. |
|
@Vipitis With that in mind I'll do another pass on the comments. I definitely think many of them are misplaced at least. |
3 participants
I'mConnections
fixes #6208
Description
Adds a
Sequenceexpression kind so side effect of comma separated expressions aren't lost.draft because I am not happy with this approach - but it seems to be working on the surface. Comments welcome this is my first time trying to fix rust code.
Testing
Adds naga glsl test case, sanity checked against shadertoy, now both are visually grey.
Squash or Rebase?
squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.