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

Treat builtins separately in Yul AST #15347

Merged
merged 6 commits into from
Dec 9, 2024
Merged

Treat builtins separately in Yul AST #15347

merged 6 commits into from
Dec 9, 2024

Conversation

clonker
Copy link
Member

@clonker clonker commented Aug 20, 2024

@clonker clonker force-pushed the builtin_ast_element branch from d93fc88 to 5f7b564 Compare August 20, 2024 07:25
@ethereum ethereum deleted a comment from stackenbotten Aug 20, 2024
@ethereum ethereum deleted a comment from stackenbotten Aug 20, 2024
@ethereum ethereum deleted a comment from stackenbotten Aug 20, 2024
@ethereum ethereum deleted a comment from stackenbotten Aug 20, 2024
@ethereum ethereum deleted a comment from stackenbotten Aug 20, 2024
@ethereum ethereum deleted a comment from stackenbotten Aug 20, 2024
@ethereum ethereum deleted a comment from stackenbotten Aug 20, 2024
@ethereum ethereum deleted a comment from stackenbotten Aug 20, 2024
@clonker clonker changed the base branch from develop to remove_yul_dialect_types August 20, 2024 11:04
@clonker clonker force-pushed the builtin_ast_element branch from 340ffbc to b69030d Compare August 20, 2024 11:05
@ethereum ethereum deleted a comment from stackenbotten Aug 20, 2024
@clonker clonker added the has dependencies The PR depends on other PRs that must be merged first label Aug 20, 2024
@ethereum ethereum deleted a comment from stackenbotten Aug 22, 2024
@ethereum ethereum deleted a comment from stackenbotten Aug 22, 2024
@clonker clonker force-pushed the builtin_ast_element branch from a5e3f71 to 52e9e4b Compare August 22, 2024 09:01
@clonker clonker force-pushed the builtin_ast_element branch 2 times, most recently from 39fdd32 to dec3a9d Compare August 26, 2024 08:17
@ethereum ethereum deleted a comment from stackenbotten Aug 26, 2024
@ethereum ethereum deleted a comment from stackenbotten Aug 26, 2024
@clonker clonker force-pushed the builtin_ast_element branch from dec3a9d to 7370d5a Compare August 26, 2024 08:21
@ethereum ethereum deleted a comment from stackenbotten Aug 26, 2024
@ethereum ethereum deleted a comment from stackenbotten Aug 26, 2024
@clonker clonker force-pushed the remove_yul_dialect_types branch 2 times, most recently from d73a5bf to ec2536e Compare August 27, 2024 08:43
@clonker clonker force-pushed the builtin_ast_element branch from 01fe1a4 to 108cf86 Compare August 27, 2024 09:14
@clonker clonker force-pushed the remove_yul_dialect_types branch 2 times, most recently from 03b01c1 to 4938075 Compare August 28, 2024 10:07
Base automatically changed from remove_yul_dialect_types to develop August 28, 2024 11:25
@cameel cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Nov 16, 2024
@ekpyron ekpyron added the 🟡 PR review label label Nov 20, 2024
@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Dec 5, 2024
@cameel

This comment was marked as resolved.

@clonker clonker removed the stale The issue/PR was marked as stale because it has been open for too long. label Dec 5, 2024
@ethereum ethereum deleted a comment from github-actions bot Dec 5, 2024
@ethereum ethereum deleted a comment from github-actions bot Dec 5, 2024
@clonker clonker force-pushed the builtin_ast_element branch from a3ef44a to 7b51864 Compare December 5, 2024 13:17
cameel
cameel previously approved these changes Dec 5, 2024
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.

Even with parts extracted into prerequisite PRs this is still a ton of code, but finally I managed to get through it all.

And it actually looks good. I didn't find any serious problems. Only a few minor things that would be good to clean up before we merge. Some of them, like CommonOptions::dialect() or YulName(string_view) or making Dialect a class, could also be done separately rather than directly in this PR.

Comment on lines 70 to 73
void InlinableExpressionFunctionFinder::checkAllowed(FunctionName _name)
{
// disallowed function names can only ever be user-defined `yul::Identifier`s, not builtins
if (std::holds_alternative<Identifier>(_name) && m_disallowedIdentifiers.count(std::get<Identifier>(_name).name))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void InlinableExpressionFunctionFinder::checkAllowed(FunctionName _name)
{
// disallowed function names can only ever be user-defined `yul::Identifier`s, not builtins
if (std::holds_alternative<Identifier>(_name) && m_disallowedIdentifiers.count(std::get<Identifier>(_name).name))
void InlinableExpressionFunctionFinder::checkAllowed(FunctionName const& _name)
{
// disallowed function names can only ever be user-defined `yul::Identifier`s, not builtins
if (std::holds_alternative<Identifier>(_name) && m_disallowedIdentifiers.count(std::get<Identifier>(_name).name) != 0)

Copy link
Member

Choose a reason for hiding this comment

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

There's also the spacing and const&. No need to make a copy of the whole AST node.

It sucks that github does not highlight all the differences in the snippet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh yeah, passing it as cref now, missed that earlier.

@clonker clonker force-pushed the builtin_ast_element branch 2 times, most recently from dda6ecd to f8fbcbd Compare December 6, 2024 06:11
@clonker clonker removed the has dependencies The PR depends on other PRs that must be merged first label Dec 6, 2024
cameel
cameel previously approved these changes Dec 6, 2024
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.

Two more places where you could use the new YulString constructor.

@clonker clonker force-pushed the builtin_ast_element branch from fca7d33 to 8994d85 Compare December 9, 2024 07:37
@clonker clonker merged commit 54efdc9 into develop Dec 9, 2024
73 checks passed
@clonker clonker deleted the builtin_ast_element branch December 9, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟡 PR review label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants