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

Differentiate between i32 and u32 in switch #2269

Merged
merged 3 commits into from
Mar 10, 2023
Merged

Differentiate between i32 and u32 in switch #2269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21b451e
Differentiate between i32 and u32 in switch
evahop Mar 8, 2023
02249d3
Use similar wording to other error messages
evahop Mar 8, 2023
99daaac
Remove duplicate enum
evahop Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/back/dot/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,8 @@ impl StatementGraph {
for case in cases {
let (case_id, case_last) = self.add(&case.body, targets);
let label = match case.value {
crate::SwitchValue::Integer(_) => "case",
crate::SwitchValue::Default => "default",
_ => "case",
};
self.flow.push((id, case_id, label));
// Link the last node of the branch to the merge node
Expand Down
12 changes: 2 additions & 10 deletions src/back/glsl/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1906,21 +1906,13 @@ impl<'a, W: Write> Writer<'a, W> {
write!(self.out, "switch(")?;
self.write_expr(selector, ctx)?;
writeln!(self.out, ") {{")?;
let type_postfix = match *ctx.info[selector].ty.inner_with(&self.module.types) {
crate::TypeInner::Scalar {
kind: crate::ScalarKind::Uint,
..
} => "u",
_ => "",
};

// Write all cases
let l2 = level.next();
for case in cases {
match case.value {
crate::SwitchValue::Integer(value) => {
write!(self.out, "{l2}case {value}{type_postfix}:")?
}
crate::SwitchValue::I32(value) => write!(self.out, "{l2}case {value}:")?,
crate::SwitchValue::U32(value) => write!(self.out, "{l2}case {value}u:")?,
crate::SwitchValue::Default => write!(self.out, "{l2}default:")?,
}

Expand Down
14 changes: 5 additions & 9 deletions src/back/hlsl/writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1883,22 +1883,18 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
write!(self.out, "switch(")?;
self.write_expr(module, selector, func_ctx)?;
writeln!(self.out, ") {{")?;
let type_postfix = match *func_ctx.info[selector].ty.inner_with(&module.types) {
crate::TypeInner::Scalar {
kind: crate::ScalarKind::Uint,
..
} => "u",
_ => "",
};

// Write all cases
let indent_level_1 = level.next();
let indent_level_2 = indent_level_1.next();

for (i, case) in cases.iter().enumerate() {
match case.value {
crate::SwitchValue::Integer(value) => {
write!(self.out, "{indent_level_1}case {value}{type_postfix}:")?
crate::SwitchValue::I32(value) => {
write!(self.out, "{indent_level_1}case {value}:")?
}
crate::SwitchValue::U32(value) => {
write!(self.out, "{indent_level_1}case {value}u:")?
}
crate::SwitchValue::Default => {
write!(self.out, "{indent_level_1}default:")?
Expand Down
14 changes: 5 additions & 9 deletions src/back/msl/writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2551,19 +2551,15 @@ impl<W: Write> Writer<W> {
} => {
write!(self.out, "{level}switch(")?;
self.put_expression(selector, &context.expression, true)?;
let type_postfix = match *context.expression.resolve_type(selector) {
crate::TypeInner::Scalar {
kind: crate::ScalarKind::Uint,
..
} => "u",
_ => "",
};
writeln!(self.out, ") {{")?;
let lcase = level.next();
for case in cases.iter() {
match case.value {
crate::SwitchValue::Integer(value) => {
write!(self.out, "{lcase}case {value}{type_postfix}:")?;
crate::SwitchValue::I32(value) => {
write!(self.out, "{lcase}case {value}:")?;
}
crate::SwitchValue::U32(value) => {
write!(self.out, "{lcase}case {value}u:")?;
}
crate::SwitchValue::Default => {
write!(self.out, "{lcase}default:")?;
Expand Down
5 changes: 4 additions & 1 deletion src/back/spv/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1770,12 +1770,15 @@ impl<'w> BlockContext<'w> {
case_ids.push(label_id);

match case.value {
crate::SwitchValue::Integer(value) => {
crate::SwitchValue::I32(value) => {
raw_cases.push(super::instructions::Case {
value: value as Word,
label_id,
});
}
crate::SwitchValue::U32(value) => {
raw_cases.push(super::instructions::Case { value, label_id });
}
crate::SwitchValue::Default => {
default_id = Some(label_id);
}
Expand Down
18 changes: 8 additions & 10 deletions src/back/wgsl/writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,14 +865,6 @@ impl<W: Write> Writer<W> {
self.write_expr(module, selector, func_ctx)?;
writeln!(self.out, " {{")?;

let type_postfix = match *func_ctx.info[selector].ty.inner_with(&module.types) {
crate::TypeInner::Scalar {
kind: crate::ScalarKind::Uint,
..
} => "u",
_ => "",
};

let l2 = level.next();
let mut new_case = true;
for case in cases {
Expand All @@ -884,11 +876,17 @@ impl<W: Write> Writer<W> {
}

match case.value {
crate::SwitchValue::Integer(value) => {
crate::SwitchValue::I32(value) => {
if new_case {
write!(self.out, "{l2}case ")?;
}
write!(self.out, "{value}")?;
}
crate::SwitchValue::U32(value) => {
if new_case {
write!(self.out, "{l2}case ")?;
}
write!(self.out, "{value}{type_postfix}")?;
write!(self.out, "{value}u")?;
}
crate::SwitchValue::Default => {
if new_case {
Expand Down
54 changes: 26 additions & 28 deletions src/front/glsl/parser/functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,35 +186,33 @@ impl<'source> ParsingContext<'source> {
let value = match self.expect_peek(frontend)?.value {
TokenValue::Case => {
self.bump(frontend)?;
let value = {
let mut stmt = ctx.stmt_ctx();
let expr = self.parse_expression(frontend, ctx, &mut stmt, body)?;
let (root, meta) =
ctx.lower_expect(stmt, frontend, expr, ExprPos::Rhs, body)?;
let constant = frontend.solve_constant(ctx, root, meta)?;

match frontend.module.constants[constant].inner {
ConstantInner::Scalar {
value: ScalarValue::Sint(int),
..
} => int as i32,
ConstantInner::Scalar {
value: ScalarValue::Uint(int),
..
} => int as i32,
_ => {
frontend.errors.push(Error {
kind: ErrorKind::SemanticError(
"Case values can only be integers".into(),
),
meta,
});

0
}

let mut stmt = ctx.stmt_ctx();
let expr = self.parse_expression(frontend, ctx, &mut stmt, body)?;
let (root, meta) =
ctx.lower_expect(stmt, frontend, expr, ExprPos::Rhs, body)?;
let constant = frontend.solve_constant(ctx, root, meta)?;

match frontend.module.constants[constant].inner {
ConstantInner::Scalar {
value: ScalarValue::Sint(int),
..
} => crate::SwitchValue::I32(int as i32),
ConstantInner::Scalar {
value: ScalarValue::Uint(int),
..
} => crate::SwitchValue::U32(int as u32),
_ => {
frontend.errors.push(Error {
kind: ErrorKind::SemanticError(
"Case values can only be integers".into(),
),
meta,
});

crate::SwitchValue::I32(0)
}
};
crate::SwitchValue::Integer(value)
}
}
TokenValue::Default => {
self.bump(frontend)?;
Expand Down
2 changes: 1 addition & 1 deletion src/front/spv/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ impl<'function> BlockContext<'function> {
let fall_through = body.last().map_or(true, |s| !s.is_terminator());

crate::SwitchCase {
value: crate::SwitchValue::Integer(value),
value: crate::SwitchValue::I32(value),
body,
fall_through,
}
Expand Down
8 changes: 4 additions & 4 deletions src/front/wgsl/lower/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1006,11 +1006,11 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
.map(|case| {
Ok(crate::SwitchCase {
value: match case.value {
ast::SwitchValue::I32(num) if !uint => {
crate::SwitchValue::Integer(num)
ast::SwitchValue::I32(value) if !uint => {
crate::SwitchValue::I32(value)
}
ast::SwitchValue::U32(num) if uint => {
crate::SwitchValue::Integer(num as i32)
ast::SwitchValue::U32(value) if uint => {
crate::SwitchValue::U32(value)
}
ast::SwitchValue::Default => crate::SwitchValue::Default,
_ => {
Expand Down
5 changes: 3 additions & 2 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1429,12 +1429,13 @@ pub use block::Block;

/// The value of the switch case.
// Clone is used only for error reporting and is not intended for end users
#[derive(Clone, Debug)]
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
#[cfg_attr(feature = "serialize", derive(Serialize))]
#[cfg_attr(feature = "deserialize", derive(Deserialize))]
#[cfg_attr(feature = "arbitrary", derive(Arbitrary))]
pub enum SwitchValue {
Integer(i32),
I32(i32),
U32(u32),
Default,
}

Expand Down
81 changes: 43 additions & 38 deletions src/valid/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,9 @@ pub enum FunctionError {
#[error("The `switch` value {0:?} is not an integer scalar")]
InvalidSwitchType(Handle<crate::Expression>),
#[error("Multiple `switch` cases for {0:?} are present")]
ConflictingSwitchCase(i32),
ConflictingSwitchCase(crate::SwitchValue),
#[error("The `switch` contains cases with conflicting types")]
ConflictingCaseType,
#[error("The `switch` is missing a `default` case")]
MissingDefaultCase,
#[error("Multiple `default` cases are present")]
Expand Down Expand Up @@ -434,52 +436,55 @@ impl super::Validator {
selector,
ref cases,
} => {
match *context.resolve_type(selector, &self.valid_expression_set)? {
Ti::Scalar {
kind: crate::ScalarKind::Uint,
width: _,
} => {}
Ti::Scalar {
kind: crate::ScalarKind::Sint,
width: _,
} => {}
let uint = match context
.resolve_type(selector, &self.valid_expression_set)?
.scalar_kind()
{
Some(crate::ScalarKind::Uint) => true,
Some(crate::ScalarKind::Sint) => false,
_ => {
return Err(FunctionError::InvalidSwitchType(selector)
.with_span_handle(selector, context.expressions))
}
}
self.select_cases.clear();
let mut default = false;
};
self.switch_values.clear();
for case in cases {
match case.value {
crate::SwitchValue::Integer(value) => {
if !self.select_cases.insert(value) {
return Err(FunctionError::ConflictingSwitchCase(value)
.with_span_static(
case.body
.span_iter()
.next()
.map_or(Default::default(), |(_, s)| *s),
"conflicting switch arm here",
));
}
}
crate::SwitchValue::Default => {
if default {
return Err(FunctionError::MultipleDefaultCases
.with_span_static(
case.body
.span_iter()
.next()
.map_or(Default::default(), |(_, s)| *s),
"duplicated switch arm here",
));
}
default = true
crate::SwitchValue::I32(_) if !uint => {}
crate::SwitchValue::U32(_) if uint => {}
crate::SwitchValue::Default => {}
_ => {
return Err(FunctionError::ConflictingCaseType.with_span_static(
case.body
.span_iter()
.next()
.map_or(Default::default(), |(_, s)| *s),
"conflicting switch arm here",
));
}
};
if !self.switch_values.insert(case.value) {
return Err(match case.value {
crate::SwitchValue::Default => FunctionError::MultipleDefaultCases
.with_span_static(
case.body
.span_iter()
.next()
.map_or(Default::default(), |(_, s)| *s),
"duplicated switch arm here",
),
_ => FunctionError::ConflictingSwitchCase(case.value)
.with_span_static(
case.body
.span_iter()
.next()
.map_or(Default::default(), |(_, s)| *s),
"conflicting switch arm here",
),
});
}
}
if !default {
if !self.switch_values.contains(&crate::SwitchValue::Default) {
return Err(FunctionError::MissingDefaultCase
.with_span_static(span, "missing default case"));
}
Expand Down
6 changes: 3 additions & 3 deletions src/valid/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ pub struct Validator {
location_mask: BitSet,
bind_group_masks: Vec<BitSet>,
#[allow(dead_code)]
select_cases: FastHashSet<i32>,
switch_values: FastHashSet<crate::SwitchValue>,
valid_expression_list: Vec<Handle<crate::Expression>>,
valid_expression_set: BitSet,
}
Expand Down Expand Up @@ -275,7 +275,7 @@ impl Validator {
layouter: Layouter::default(),
location_mask: BitSet::new(),
bind_group_masks: Vec::new(),
select_cases: FastHashSet::default(),
switch_values: FastHashSet::default(),
valid_expression_list: Vec::new(),
valid_expression_set: BitSet::new(),
}
Expand All @@ -287,7 +287,7 @@ impl Validator {
self.layouter.clear();
self.location_mask.clear();
self.bind_group_masks.clear();
self.select_cases.clear();
self.switch_values.clear();
self.valid_expression_list.clear();
self.valid_expression_set.clear();
}
Expand Down