Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle empty variable names in namer #1484

Merged
merged 2 commits into from
Oct 26, 2021
Merged

Handle empty variable names in namer #1484

merged 2 commits into from
Oct 26, 2021

Conversation

Gordon-F
Copy link
Collaborator

Fix #1483

@kvark kvark merged commit 00bbbed into gfx-rs:master Oct 26, 2021
@kvark kvark added the can backport PR that can be back-ported to a release branch label Oct 26, 2021
@Gordon-F Gordon-F deleted the handle_empty_variables branch October 27, 2021 08:39
@jimblandy
Copy link
Member

jimblandy commented Oct 31, 2021 •
edited
Loading

The test case in this PR passes even without the patch. The unnamed uniform has a name of None, and Namer::reset supplies "global" as the fallback in that case.

@jimblandy
Copy link
Member

The .spv file provided in #1483 does trigger the bug, though.

@jimblandy
Copy link
Member

The fix in the PR, however, is correct: the SPIR-V file provokes a call like this:

call_or(Some(""), "global") -> ""

The Some("") case is exactly what the PR handles.

It's just the test case that is ineffectual.

@Gordon-F
Copy link
Collaborator Author

The test case in this PR passes even without the patch. The unnamed uniform has a name of None, and Namer::reset supplies "global" as the fallback in that case.

Yes, you are right, thank you. It's because glsl-in parse empty name as None, but spv-in parse as Some(""). I will make a PR with spv test instead of glsl.

kvark pushed a commit to kvark/naga that referenced this pull request Dec 2, 2021
@Gordon-F @kvark
Handle empty variable names in namer (gfx-rs#1484)
1eb33be 
* Handle empty variable names in namer

* Add glsl-in test with empty global name
@kvark kvark mentioned this pull request Dec 2, 2021
Merged
@kvark kvark removed the can backport PR that can be back-ported to a release branch label Dec 2, 2021
kvark pushed a commit that referenced this pull request Dec 2, 2021
@Gordon-F @kvark
Handle empty variable names in namer (#1484)
469ebc0 
* Handle empty variable names in namer

* Add glsl-in test with empty global name
@kvark
Copy link
Member

kvark commented Dec 2, 2021

Published in 0.7.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvark kvark kvark approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[wgsl-out] Certain SPIR-V Patterns Cause Invalid Accesses
3 participants
@Gordon-F @jimblandy @kvark