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

Avoid pushing zero-sized variables. #4275

Closed
wants to merge 2 commits into from

Conversation

ilyalesokhin-starkware
Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware commented Oct 18, 2023

This change is Reviewable

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: 0 of 32 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-starknet/test_data/new_syntax_test_contract__counter_contract.sierra line 1658 at r1 (raw file):

rename<Array<felt252>>([7]) -> ([10]); // 1328
rename<Array<felt252>>([8]) -> ([11]); // 1329
jump() { 1365() }; // 1330

the store_temps before the jump were changed to renames and now the code fails sierra to casm.

Code quote:

function_call<user@cairo_level_tests::contracts::new_syntax_test_contract::test_component::EventIsEvent::append_keys_and_data>([3], [1], [2]) -> ([7], [8], [9]); // 1326
drop<Unit>([9]) -> (); // 1327
rename<Array<felt252>>([7]) -> ([10]); // 1328
rename<Array<felt252>>([8]) -> ([11]); // 1329
jump() { 1365() }; // 1330

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: 0 of 32 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-starknet/test_data/new_syntax_test_contract__counter_contract.sierra line 1658 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

the store_temps before the jump were changed to renames and now the code fails sierra to casm.

See
https://reviewable.io/reviews/starkware-libs/cairo/4276

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 31 of 32 files at r1, 21 of 21 files at r2, all commit messages.
Reviewable status: 52 of 53 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-starknet/test_data/new_syntax_test_contract__counter_contract.sierra line 1658 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

See
https://reviewable.io/reviews/starkware-libs/cairo/4276

so why don't i see the tests failing?

@ilyalesokhin-starkware
Copy link
Contributor Author

crates/cairo-lang-starknet/test_data/new_syntax_test_contract__counter_contract.sierra line 1658 at r1 (raw file):

Previously, orizi wrote…

so why don't i see the tests failing?

I'ved rebased this on top of the fix.
you can look at the previous ci run.

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 32 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

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.

We need to merge
https://reviewable.io/reviews/starkware-libs/cairo/4276#-
first.
Note that is changes sierra_to_casm.

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

@ilyalesokhin-starkware ilyalesokhin-starkware marked this pull request as draft October 18, 2023 18:15
@ilyalesokhin-starkware
Copy link
Contributor Author

ilyalesokhin-starkware commented Oct 23, 2023

Replaced by #4290

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.

2 participants