Skip to content

Treat builtins separately in Yul AST#15347

Merged
clonker merged 6 commits intodevelopfrom
builtin_ast_element
Dec 9, 2024
Merged

Treat builtins separately in Yul AST#15347
clonker merged 6 commits intodevelopfrom
builtin_ast_element

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
@argotorg argotorg deleted a comment from stackenbotten Aug 20, 2024
@argotorg argotorg deleted a comment from stackenbotten Aug 20, 2024
@argotorg argotorg deleted a comment from stackenbotten Aug 20, 2024
@argotorg argotorg deleted a comment from stackenbotten Aug 20, 2024
@argotorg argotorg deleted a comment from stackenbotten Aug 20, 2024
@argotorg argotorg deleted a comment from stackenbotten Aug 20, 2024
@argotorg argotorg deleted a comment from stackenbotten Aug 20, 2024
@argotorg argotorg 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
@argotorg argotorg 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
@argotorg argotorg deleted a comment from stackenbotten Aug 22, 2024
@argotorg argotorg 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
@argotorg argotorg deleted a comment from stackenbotten Aug 26, 2024
@argotorg argotorg 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
@argotorg argotorg deleted a comment from stackenbotten Aug 26, 2024
@argotorg argotorg 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
@clonker clonker force-pushed the yul_object_contains_dialect branch from a59ec52 to 371647c Compare October 31, 2024 09:23
Base automatically changed from yul_object_contains_dialect to develop October 31, 2024 16:46
@clonker clonker force-pushed the builtin_ast_element branch from ea36743 to a3ef44a Compare November 1, 2024 09:11
@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Nov 15, 2024
@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
@argotorg argotorg deleted a comment from github-actions bot Dec 5, 2024
@argotorg argotorg 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
Collaborator

@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
Collaborator

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
Collaborator

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
Collaborator

@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.

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