-
Notifications
You must be signed in to change notification settings - Fork 18
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
fix: the try-catch lowering hack for 0.8.25 #549
Conversation
This patch moves the success tag tracking of the try-call ast to its annotation instead of CompilerContext. This fixes the lowering of nested try-catch statements. This fixes CPR-1705.
This should help with the rebase onto older releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
Could you update the changelog? Assume it's a version 1.0.1
Done. |
…mpilererror-in-our-fork
Benchmark results:
|
@antonbaliasnikov is there ZKSYNC_VERSION set for the reference build? |
Yeah, this was due to last second changes in |
New results for the reference, all looks good now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge when reference passes.
The reference does not contain this patch, so it failed with:
|
Please check DM in Slack! |
Reference is expected to fail right? The reference solc binary doesn't have this patch that would fix the compilation of try_catch/nested_2.sol, right? |
Nope. Reference has always supposed to pass, as it contains neither the fix nor the reproducer to fail. Its only purpose is to produce a benchmark for candidate to compare with. |
@antonbaliasnikov thanks. Let's have CI changes in one commit. |
@abinavpp after rebase, the changelog should go away and we'll end up with two commits: the fix and the CI. |
What ❔
Fixes the try-catch lowering hack.
Why ❔
CPR-1705
Checklist