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

Added panic_with_const_felt252 function and using it where possible. #7386

Merged
merged 1 commit into from
Mar 5, 2025

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Mar 4, 2025

Using it by:

  • const-folding panic_with_felt252 into it.
  • panic_with using it directly.
  • gas addition using it directly.

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

@reviewable-StarkWare
Copy link

This change is Reviewable

@orizi orizi requested a review from gilbens-starkware March 4, 2025 10:58
@orizi orizi force-pushed the spr/main/37a833cf branch from 62c64ce to c1344eb Compare March 4, 2025 11:17
@orizi orizi force-pushed the spr/main/b187cb5a branch from cb1b40d to 93f7379 Compare March 4, 2025 11:29
@orizi orizi force-pushed the spr/main/37a833cf branch 2 times, most recently from e2d5911 to 06cf6b9 Compare March 4, 2025 11:35
@orizi orizi force-pushed the spr/main/b187cb5a branch 2 times, most recently from 0a40007 to e01f69d Compare March 4, 2025 11:37
@orizi orizi force-pushed the spr/main/37a833cf branch 2 times, most recently from 1a2d88f to 82c0320 Compare March 4, 2025 11:58
@orizi orizi force-pushed the spr/main/b187cb5a branch 2 times, most recently from e5a9235 to 35d0e55 Compare March 4, 2025 12:12
@orizi orizi force-pushed the spr/main/37a833cf branch from 82c0320 to 59e5595 Compare March 4, 2025 12:12
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 91 files at r1, all commit messages.
Reviewable status: 2 of 91 files reviewed, 1 unresolved discussion


a discussion (no related file):
Won't it create an instance of the function for each const and thus potentially increase the code size?

@orizi orizi force-pushed the spr/main/b187cb5a branch from 35d0e55 to 19f5616 Compare March 4, 2025 13:25
@orizi orizi force-pushed the spr/main/37a833cf branch from 59e5595 to 9c01b83 Compare March 4, 2025 13:26
Copy link
Collaborator Author

@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 91 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware)


a discussion (no related file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Won't it create an instance of the function for each const and thus potentially increase the code size?

potentially - yes - in actuality - no.

most shorterrors repeat themselves - and we specifically don't care much about run cost of the panic area of the code (as it shouldn't happen)

@orizi orizi force-pushed the spr/main/37a833cf branch from 9c01b83 to 9490257 Compare March 4, 2025 13:51
@orizi orizi changed the base branch from spr/main/b187cb5a to main March 4, 2025 13:51
@orizi orizi enabled auto-merge March 4, 2025 14:25
@orizi orizi force-pushed the spr/main/37a833cf branch from 9490257 to ba7c424 Compare March 4, 2025 15:19
Using it by:
* const-folding `panic_with_felt252` into it.
* panic_with using it directly.
* gas addition using it directly.

commit-id:37a833cf
@orizi orizi force-pushed the spr/main/37a833cf branch from ba7c424 to fd3e538 Compare March 4, 2025 17:29
Copy link
Contributor

@gilbens-starkware gilbens-starkware 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 56 of 91 files at r1, 33 of 33 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

@orizi orizi added this pull request to the merge queue Mar 5, 2025
Merged via the queue into main with commit ff0ae55 Mar 5, 2025
92 checks passed
@orizi orizi deleted the spr/main/37a833cf branch March 5, 2025 09:34
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