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

Yul: Immutable AST class with top-level block #15258

Merged
merged 5 commits into from
Aug 6, 2024
Merged

Conversation

clonker
Copy link
Member

@clonker clonker commented Jul 11, 2024

@ekpyron
Copy link
Member

ekpyron commented Jul 22, 2024

Looks like this is in dire need of a rebase

@clonker clonker force-pushed the immutable_ast_class branch 2 times, most recently from b1ae6a5 to 4247ad4 Compare July 22, 2024 11:36
@clonker clonker requested a review from r0qs July 22, 2024 13:36
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

No issues on terms of correctness, but could use better naming and the override in compilability checker could be done differently. See comments.

libyul/AST.h Outdated Show resolved Hide resolved
libyul/AsmAnalysis.h Outdated Show resolved Hide resolved
libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved
libyul/CompilabilityChecker.h Outdated Show resolved Hide resolved
libyul/optimiser/StackLimitEvader.h Outdated Show resolved Hide resolved
libsolidity/ast/AST.h Show resolved Hide resolved
libyul/optimiser/Suite.cpp Outdated Show resolved Hide resolved
test/libyul/YulOptimizerTestCommon.h Outdated Show resolved Hide resolved
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Unfortunately I did find some new stuff on this second pass:

  • I think we should move AsmAnalysisInfo to the AST struct and make it immutable as well. It would reduce the risk of forgetting to update it and we would also be able to calculate it just once per AST.
  • YulOptimizerTest allows arbitrary object nesting but does not handle that properly (only cares about the top-level one). And now with the extra copying added by this PR that gets extra unsafe, because we're not doing full copies of the objects. We should safeguard that with some asserts and maybe consider also actually fixing it properly (later, in a separate PR).

test/libyul/YulOptimizerTestCommon.cpp Outdated Show resolved Hide resolved
test/libyul/YulOptimizerTestCommon.cpp Outdated Show resolved Hide resolved
libyul/optimiser/StackCompressor.cpp Outdated Show resolved Hide resolved
libyul/Object.cpp Outdated Show resolved Hide resolved
@clonker clonker force-pushed the immutable_ast_class branch 2 times, most recently from ae6b298 to ca2eabe Compare August 2, 2024 14:10
libyul/Object.cpp Show resolved Hide resolved
libyul/Object.cpp Outdated Show resolved Hide resolved
@ekpyron ekpyron enabled auto-merge August 6, 2024 14:33
@ekpyron ekpyron merged commit c92b32f into develop Aug 6, 2024
70 of 71 checks passed
@ekpyron ekpyron deleted the immutable_ast_class branch August 6, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants