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.

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

Invalidate snapped_semantics. #7272

Merged
merged 1 commit into from
Feb 13, 2025
Merged

Conversation

ilyalesokhin-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@ilyalesokhin-starkware
Copy link
Contributor Author

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

  (v42: core::panics::PanicResult::<(core::integer::u32,)>) <- PanicResult::Err(v28)
End:
  Return(v24, v25, v42)

without the fix

Suggestion:

blk2:
Statements:
  (v34: core::integer::u32) <- core::array::array_len::<core::integer::u32>(v22)
  (v35: (core::integer::u32,)) <- struct_construct(v34)
  (v36: core::panics::PanicResult::<(core::integer::u32,)>) <- PanicResult::Ok(v35)
End:
  Return(v29, v30, v36)

blk3:
Statements:
  (v37: core::panics::PanicResult::<(core::integer::u32,)>) <- PanicResult::Err(v33)
End:
  Return(v29, v30, v37)

blk4:
Statements:
  (v38: core::panics::PanicResult::<(core::integer::u32,)>) <- PanicResult::Err(v28)
End:
  Return(v24, v25, v38)

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 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @TomerStarkware)


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

            .add(ctx, &mut builder.statements);
            builder.update_ref(ctx, snapshot_param, original);
            builder.update_snap_ref(snapshot_param, snapped);

add doc about the ordering of these lines.

Code quote:

            builder.update_snap_ref(snapshot_param, snapped);

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

        self.changed_member_paths.insert(member_path);
    
    }

Suggestion:

        self.changed_member_paths.insert(member_path);
    }

Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-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: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @orizi and @TomerStarkware)


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

Previously, orizi wrote…

add doc about the ordering of these lines.

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.

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)


crates/cairo-lang-lowering/src/lower/mod.rs line 1521 at r3 (raw file):

                    // TODO(TomerStaskware): make sure this is unreachable and remove
                    // `MemberPathLoop` diagnostic.
                    LoweringFlowError::Failed(ctx.diagnostics.report(stable_ptr, MemberPathLoop))

How's this related?

@ilyalesokhin-starkware
Copy link
Contributor Author

crates/cairo-lang-lowering/src/lower/mod.rs line 1521 at r3 (raw file):

Previously, orizi wrote…

How's this related?

the error location was kind of useless without this fix.
do you want this as a separate PR?

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

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.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-lowering/src/test_data/snapshot line 249 at r3 (raw file):

    }

    clear_scores(ref array1);

array.pop_front().unwrap();
is enough
or even array1 = array![]

Code quote:

clear_scores(ref array1);

Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-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: all files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)


crates/cairo-lang-lowering/src/test_data/snapshot line 249 at r3 (raw file):

Previously, TomerStarkware wrote…

array.pop_front().unwrap();
is enough
or even array1 = array![]

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.

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

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:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

@ilyalesokhin-starkware ilyalesokhin-starkware added this pull request to the merge queue Feb 13, 2025
Merged via the queue into main with commit dd42945 Feb 13, 2025
47 checks passed
orizi pushed a commit that referenced this pull request Feb 13, 2025
@orizi orizi deleted the ilya/invalidate_snap_ref branch February 16, 2025 07:04
orizi pushed a commit that referenced this pull request Feb 16, 2025
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