Skip to content

flow-control: Add an if-based numeric match.#8130

Merged
liorgold2 merged 1 commit intomainfrom
pr/liorgold2/lior/flow-control/eaebd1fa
Aug 10, 2025
Merged

flow-control: Add an if-based numeric match.#8130
liorgold2 merged 1 commit intomainfrom
pr/liorgold2/lior/flow-control/eaebd1fa

Conversation

@liorgold2
Copy link
Collaborator

@liorgold2 liorgold2 commented Aug 9, 2025

@reviewable-StarkWare
Copy link

This change is Reviewable

@liorgold2 liorgold2 requested a review from orizi August 10, 2025 09:51
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 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions


crates/cairo-lang-lowering/src/lower/flow_control/create_graph/patterns.rs line 323 at r1 (raw file):

                // Extract the literal value as `usize`.
                let Some(literal) = literal.value.to_usize() else {
                    todo!();

what is the todo? check if unreachable? add diagnostic?

Code quote:

                    todo!();

crates/cairo-lang-lowering/src/lower/flow_control/create_graph/patterns.rs line 334 at r1 (raw file):

                }
            }
            _ => todo!(),

ditto.

Code quote:

            _ => todo!(),

crates/cairo-lang-lowering/src/lower/flow_control/test_data/match line 399 at r1 (raw file):

        0 => 0,
        _ => 1,
    }

you plan to support this despite it not being contiguous? (so in this case - without 1 as one of the options)

Code quote:

    match x {
        2 | 4 => x,
        0 => 0,
        _ => 1,
    }

@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/cedc4df8 branch from 810b876 to c7d41a2 Compare August 10, 2025 10:49
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/eaebd1fa branch 2 times, most recently from dd9f730 to 0222f09 Compare August 10, 2025 11:37
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/cedc4df8 branch from c7d41a2 to ceed3f7 Compare August 10, 2025 11:37
Copy link
Collaborator Author

@liorgold2 liorgold2 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: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @orizi)


crates/cairo-lang-lowering/src/lower/flow_control/create_graph/patterns.rs line 323 at r1 (raw file):

Previously, orizi wrote…

what is the todo? check if unreachable? add diagnostic?

Sorry, I got lazy. Added a TODO comment.


crates/cairo-lang-lowering/src/lower/flow_control/create_graph/patterns.rs line 334 at r1 (raw file):

Previously, orizi wrote…

ditto.

Done
(Variable is unreachable, the rest should be diagnostics.)


crates/cairo-lang-lowering/src/lower/flow_control/test_data/match line 399 at r1 (raw file):

Previously, orizi wrote…

you plan to support this despite it not being contiguous? (so in this case - without 1 as one of the options)

Yes, I don't see why not to support it.

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 r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @liorgold2)


crates/cairo-lang-lowering/src/lower/flow_control/test_data/match line 399 at r1 (raw file):

Previously, liorgold2 wrote…

Yes, I don't see why not to support it.

my main concern is that non-consecutive match will be in efficient - but i guess this is lintable.

@liorgold2 liorgold2 changed the base branch from pr/liorgold2/lior/flow-control/cedc4df8 to main August 10, 2025 12:00
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/eaebd1fa branch from 0222f09 to 1792553 Compare August 10, 2025 12:00
Copy link
Collaborator Author

@liorgold2 liorgold2 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @liorgold2)


crates/cairo-lang-lowering/src/lower/flow_control/test_data/match line 399 at r1 (raw file):

Previously, orizi wrote…

my main concern is that non-consecutive match will be in efficient - but i guess this is lintable.

Yes, this should be checked in the linter, not be a restriction to the compilation.

@liorgold2 liorgold2 added this pull request to the merge queue Aug 10, 2025
Merged via the queue into main with commit 221330b Aug 10, 2025
96 checks passed
@orizi orizi deleted the pr/liorgold2/lior/flow-control/eaebd1fa branch August 11, 2025 14:13
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.

3 participants