Skip to content

Conversation

liorgold2
Copy link
Collaborator

@liorgold2 liorgold2 commented Jul 31, 2025

@reviewable-StarkWare
Copy link

This change is Reviewable

@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/97600111 branch 2 times, most recently from fdcebc2 to ad6a371 Compare July 31, 2025 16:45
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/d1a5121c branch from ed018b0 to b694dae Compare August 1, 2025 09:41
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/97600111 branch from ad6a371 to f6296a6 Compare August 1, 2025 09:41
@liorgold2 liorgold2 requested a review from orizi August 3, 2025 06:27
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 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @liorgold2)


crates/cairo-lang-lowering/src/borrow_check/test_data/borrow_check line 116 at r1 (raw file):

| ...
|     } else {}
|_____________^

this is somewhat better now - right?

Code quote:

  --> lib.cairo:9:5-12:13
      if true {
 _____^
| ...
|     } else {}
|_____________^

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

/// In particular, note that if `conditions` is empty, there are no conditions and the
/// expression is simply [Self::expr].
pub struct ConditionedExpr<'db, 'a> {

you can probably merge lifetimes - as the conditions vector originates from the db as well.

Suggestion:

pub struct ConditionedExpr<'db> {

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: all files reviewed, 2 unresolved discussions (waiting on @orizi)


crates/cairo-lang-lowering/src/borrow_check/test_data/borrow_check line 116 at r1 (raw file):

Previously, orizi wrote…

this is somewhat better now - right?

Yes. In fact, this is the way it was originally (I changed it to point to the condition when I did the let-chains).


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

Previously, orizi wrote…

you can probably merge lifetimes - as the conditions vector originates from the db as well.

Not sure -
this is how I create the struct:

        &ConditionedExpr {
            expr: expr.if_block,
            conditions: &expr.conditions,
            else_block: expr.else_block,
            if_expr_location: ctx.get_location(expr.stable_ptr.untyped()),
        },

and since conditions is stored without a reference in expr, the lifetime of &expr.conditions is the same as the lifetime of expr, which is defined as expr: &semantic::ExprIf<'db> - so it's an anonymous lifetime and not 'db. I haven't looked into changing this as well.

(anyway, since this file should eventually be removed, I don't want to invest too much time into it, doing it as an exercise with lifetimes)

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:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @liorgold2)

@liorgold2 liorgold2 changed the title flow_control: Small changes in lower_if_bool_condition to align with the new code. flow_control: Small changes in lower_if_bool_condition to align with … Aug 3, 2025
@liorgold2 liorgold2 changed the base branch from pr/liorgold2/lior/flow-control/d1a5121c to main August 3, 2025 07:53
@liorgold2 liorgold2 changed the title flow_control: Small changes in lower_if_bool_condition to align with … flow_control: Small changes in lower_if_bool_condition to align with the new code. Aug 3, 2025
@liorgold2 liorgold2 force-pushed the pr/liorgold2/lior/flow-control/97600111 branch from f6296a6 to a6bc71d Compare August 3, 2025 07:53
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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @liorgold2)

@liorgold2 liorgold2 enabled auto-merge August 3, 2025 08:00
@liorgold2 liorgold2 added this pull request to the merge queue Aug 3, 2025
Merged via the queue into main with commit 081f8ab Aug 3, 2025
96 checks passed
@orizi orizi deleted the pr/liorgold2/lior/flow-control/97600111 branch August 4, 2025 11:48
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