-
Notifications
You must be signed in to change notification settings - Fork 596
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
support for match out of order for enums #4536
Conversation
5660789
to
53bf8c7
Compare
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.
Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @TomerStarkware)
crates/cairo-lang-lowering/src/lower/mod.rs
line 1032 at r2 (raw file):
}) .collect(); let variant_map = variant_map?;
Suggestion:
Ok((enum_pattern.variant.clone(), (enum_pattern, arm_index)))
})
.collect::<Result<Vec<_>,_>>()?;
crates/cairo-lang-lowering/src/lower/mod.rs
line 1117 at r2 (raw file):
)); } // Merge arm blocks.
is this exactly the same?
extract to function.
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.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @TomerStarkware)
crates/cairo-lang-lowering/src/test_data/match
line 485 at r2 (raw file):
//! > ========================================================================== //! > Test out of order extern match arm.
is there a place testing non-diagnostics?
is it this file?
Code quote:
Test out of order extern match arm.
418007a
to
a91175e
Compare
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.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-lowering/src/lower/mod.rs
line 1032 at r2 (raw file):
}) .collect(); let variant_map = variant_map?;
Done.
crates/cairo-lang-lowering/src/lower/mod.rs
line 1117 at r2 (raw file):
Previously, orizi wrote…
is this exactly the same?
extract to function.
Done.
crates/cairo-lang-lowering/src/test_data/match
line 485 at r2 (raw file):
Previously, orizi wrote…
is there a place testing non-diagnostics?
is it this file?
add number to differentiate the blocks
a91175e
to
5462204
Compare
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.
Reviewed 1 of 2 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @TomerStarkware)
crates/cairo-lang-lowering/src/lower/mod.rs
line 983 at r4 (raw file):
} // get an unordered map of variants to their corresponding arm index
Suggestion:
/// Returns a map from variants to their corresponding arm index.
crates/cairo-lang-lowering/src/lower/mod.rs
line 1011 at r4 (raw file):
Ok((enum_pattern.variant.clone(), (enum_pattern, arm_index))) }) .collect::<Result<UnorderedHashMap<_, _>, _>>()
Is this required? (due to the function ret value)
Suggestion:
Ok((enum_pattern.variant.clone(), (enum_pattern, arm_index)))
})
.collect()
examples/fib.cairo
line 13 at r4 (raw file):
A::One(_) => { 1 }, } }
remove
Code quote:
fn foo(a: A) -> felt252 {
match a {
A::Two(_) => { 2 },
A::One(_) => { 1 },
}
}
ba5be18
to
db8a9be
Compare
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.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-lowering/src/lower/mod.rs
line 983 at r4 (raw file):
} // get an unordered map of variants to their corresponding arm index
Done.
crates/cairo-lang-lowering/src/lower/mod.rs
line 1011 at r4 (raw file):
Previously, orizi wrote…
Is this required? (due to the function ret value)
Done.
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.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware)
db8a9be
to
d626105
Compare
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.
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware)
This change is