-
Notifications
You must be signed in to change notification settings - Fork 193
[hlsl-out] Fix fallthrough in switch statements (for FXC) #1920
Conversation
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.
Looks good. Just one suggestion.
let curr_len = i + 1; | ||
let end_case_idx = curr_len | ||
+ cases | ||
.iter() | ||
.skip(curr_len) | ||
.position(|case| !case.fall_through) | ||
.unwrap(); | ||
let indent_level_3 = indent_level_2.next(); | ||
for case in &cases[i..=end_case_idx] { | ||
writeln!(self.out, "{}{{", indent_level_2)?; | ||
for sta in case.body.iter() { | ||
self.write_stmt(module, sta, func_ctx, indent_level_3)?; | ||
} | ||
writeln!(self.out, "{}}}", indent_level_2)?; | ||
} |
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 think this could be simplified a bit:
let curr_len = i + 1; | |
let end_case_idx = curr_len | |
+ cases | |
.iter() | |
.skip(curr_len) | |
.position(|case| !case.fall_through) | |
.unwrap(); | |
let indent_level_3 = indent_level_2.next(); | |
for case in &cases[i..=end_case_idx] { | |
writeln!(self.out, "{}{{", indent_level_2)?; | |
for sta in case.body.iter() { | |
self.write_stmt(module, sta, func_ctx, indent_level_3)?; | |
} | |
writeln!(self.out, "{}}}", indent_level_2)?; | |
} | |
let rest = &cases[i..]; | |
let fallthrough_count = rest | |
.iter() | |
.position(|case| !case.fall_through) | |
.unwrap(); | |
// Include the final, non-`fall_through` case. | |
let fallthrough = &rest[..=fallthrough_count]; | |
let indent_level_3 = indent_level_2.next(); | |
for case in fallthrough { | |
writeln!(self.out, "{}{{", indent_level_2)?; | |
for sta in case.body.iter() { | |
self.write_stmt(module, sta, func_ctx, indent_level_3)?; | |
} | |
writeln!(self.out, "{}}}", indent_level_2)?; | |
} |
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 find the naming to be slightly more confusing here since rest
and fallthrough
contain the first case as well. Getting fallthrough_count
should also skip the first element (since it's the first case) and add one at the end.
I tried to make it more clear as well without much luck...
let last_case = &cases[end_case_idx]; | ||
if last_case.body.last().map_or(true, |s| !s.is_terminator()) { | ||
writeln!(self.out, "{}break;", indent_level_2)?; | ||
} |
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.
Pair with the above suggestion:
let last_case = &cases[end_case_idx]; | |
if last_case.body.last().map_or(true, |s| !s.is_terminator()) { | |
writeln!(self.out, "{}break;", indent_level_2)?; | |
} | |
let last_case = &fallthrough[fallthrough_count]; | |
if last_case.body.last().map_or(true, |s| !s.is_terminator()) { | |
writeln!(self.out, "{}break;", indent_level_2)?; | |
} |
You know, I should just keep minor style stuff like that to myself. Merging as posted. |
Fixes FXC
error X3537: Fall-throughs in switch statements are not allowed
by emitting the statements of the following case blocks (the ones the fallthrough was supposed to reach).