Skip to content

Represent match arm wrapper as an enum with explicit cases #7726

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

Merged
merged 1 commit into from
May 8, 2025

Conversation

eytan-starkware
Copy link
Contributor

@eytan-starkware eytan-starkware commented May 6, 2025

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a 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 r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TomerStarkware)


crates/cairo-lang-lowering/src/lower/lower_match.rs line 52 at r1 (raw file):

/// MatchArm wrapper that allows for:
/// regular match patterns, else clause, and default (no else) clauses.

rephrase - the doc above should describe what is the meaning of the enum - description of the variants should be lest at the variants.

Code quote:

/// MatchArm wrapper that allows for:
/// regular match patterns, else clause, and default (no else) clauses.

crates/cairo-lang-lowering/src/lower/lower_match.rs line 672 at r1 (raw file):

    let pattern = pattern_path
        .pattern_index
        .map(|i| arms[pattern_path.arm_index].pattern(ctx, i).unwrap().clone());

Suggestion:

    let pattern = pattern_path
        .pattern_index
        .and(|i| arms[pattern_path.arm_index].pattern(ctx, i).cloned());

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @orizi and @TomerStarkware)


crates/cairo-lang-lowering/src/lower/lower_match.rs line 52 at r1 (raw file):

Previously, orizi wrote…

rephrase - the doc above should describe what is the meaning of the enum - description of the variants should be lest at the variants.

Done.


crates/cairo-lang-lowering/src/lower/lower_match.rs line 672 at r1 (raw file):

    let pattern = pattern_path
        .pattern_index
        .map(|i| arms[pattern_path.arm_index].pattern(ctx, i).unwrap().clone());

I tried to preserve a compiler crash if we don't have a pattern when the index is available. That is, we previously found the pattern at this arm, but now it doesn't exist. Do we want to change that case to continue as if pattern is None?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @eytan-starkware and @TomerStarkware)


crates/cairo-lang-lowering/src/lower/lower_match.rs line 672 at r1 (raw file):

Previously, eytan-starkware wrote…

I tried to preserve a compiler crash if we don't have a pattern when the index is available. That is, we previously found the pattern at this arm, but now it doesn't exist. Do we want to change that case to continue as if pattern is None?

rather not have explicit unwraps - if you insist - add an expect.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eytan-starkware and @TomerStarkware)

Copy link
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @orizi and @TomerStarkware)


crates/cairo-lang-lowering/src/lower/lower_match.rs line 672 at r1 (raw file):

Previously, orizi wrote…

rather not have explicit unwraps - if you insist - add an expect.

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TomerStarkware)

Base automatically changed from spr/main/4d872b2d to main May 8, 2025 09:40
Copy link
Collaborator

@TomerStarkware TomerStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed all commit messages.
Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @eytan-starkware)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @eytan-starkware)

@eytan-starkware eytan-starkware added this pull request to the merge queue May 8, 2025
Merged via the queue into main with commit 1732748 May 8, 2025
49 checks passed
@eytan-starkware eytan-starkware deleted the spr/main/415bf8b4 branch May 8, 2025 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants