Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Switch statement improvements #2126
Switch statement improvements #2126
Changes from all commits
e55823d
5e99640
d1948e3
69342d1
5c87a4e
1880404
833ce7d
e9f817d
cbf84b9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: It's a bummer that we're not removing
fall_through
, too; the complexity. From our sync conversation, you mentioned that it wasn't feasible to remove at this time, because IR needs to keep it for other front ends (IIRC?). Is there an issue to track this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That but also the fact that we still translate a switch with multiple cases with the same body as a sequence of switch cases with no body and
fall_through = true
ending in a case with the body andfall_through = false
.i.e.
V
I'll add some more docs on the Switch statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idea(non-blocking): Maybe we could change
SwitchCase
model to removefall_through
and makebody
anOption
? Concretely:A one-off
enum
might also serve well here, but I'm less certain about that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other frontends still make use of fall_through with non empty bodies though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code that decides when to write "case" is fine, but it does seem like the IR is not making the writer's life easy. We might be able to improve the IR, but that should be a separate PR.
Suggestion: Would it work to do something like this?
(I'm just making this up, please check my logic carefully!)
Granted, this would need to be changed when we implement fallthrough, but I think if we trade
split_inclusive
for.enumerate().filter()
and then gather up all the consecutive bodies we need to concatenate, we could keep this as a loop whose iterations correspond to emitted "case" statements, not IR cases, which is what I think makes it clearer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this but I could not come up with a satisfactory change because we still need to support "real" fall-through for GLSL and SPV.
I don't see how
.enumerate().filter()
would work since we still need to emit the case values of thefall_through
cases. As far as I can tell the only way to do this would be to allocate a new structure and have one pass restructuring the data then another to write it which I don't think is really worth it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: I know that in
wgpu
, cwfitzgerald requests thatTODO
s be either resolved or converted to issues. I see that there are lots of otherTODO
s in the repo, currently; doesnaga
maintainership take a stance on "WIP"-ish comments?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hasn't been brought up as far as I'm aware. It would be nice but I think we should do a round of converting all
TODO
s to issues first (which has been on my mind for a while but not a priority currently).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't introduce any new TODO comments to Naga. The issue tracker is a much better tool for keeping track of these sorts of plans and ideas, so please file an issue for this. (Instead of a "TODO" comment, a link to an issue is fine, if you think it would be helpful to future readers.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, filed https://github.com/gfx-rs/naga/issues/2166.