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

Inline the (Yul)Statement parser in v0/v1 #658

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Nov 15, 2023

Part of #652

Seems convenient for the AST/CST (to not need to unwrap another layer) but we lose the ability to test it as a named parser and the generated parser code gets a bit bloated, since we now inline the choice.

Maybe it'd be better if we somehow allow to "outline" select enums in v2, like this one? @OmarTawfik

@Xanewok Xanewok requested a review from a team as a code owner November 15, 2023 15:34
Copy link

changeset-bot bot commented Nov 15, 2023

⚠️ No Changeset found

Latest commit: 3510c00

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR


Errors: # 1 total
- >
Error: Expected end of file.
Copy link
Contributor

Choose a reason for hiding this comment

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

This side effect is also true for Precedence items, and for the same reason, since now they always result in a unique node (an operator expression, or a primary expression), they don't have to produce their own top-level nodes. From the design doc:

It is important to note that the item name doesn't contribute a NonTerminalKind, but each PrecedenceExpression under it contributes one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate a bit which side effect are you referring to here?

Copy link
Contributor

@OmarTawfik OmarTawfik Nov 20, 2023

Choose a reason for hiding this comment

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

I was commenting on:

Maybe it'd be better if we somehow allow to "outline" select enums in v2, like this one? @OmarTawfik

Are you suggesting this just for testing? I think I will have to introduce strongly typed parseEnumFoo() -> EnumFoo at some point anyways. We can test its root node, even if it wasn't wrapped with an explicit NonTerminalKind. Thoughts?

I was also mentioning that like Enum items, Precedence items also don't produce their own NonTerminalKind, but their inner precedence_expressions entries do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I wanted to specifically say that being able to parse a general construct like "expression" or "statement" seems like something we want to do anyway, even if it doesn't have a dedicated rule kind in the CST (is inlined), so the future parseEnum approach answers my concern.

@Xanewok Xanewok added this pull request to the merge queue Nov 15, 2023
Merged via the queue into NomicFoundation:main with commit 3e4d2b6 Nov 15, 2023
1 check passed
@Xanewok Xanewok deleted the outline-statements branch November 15, 2023 17:42
@Xanewok Xanewok mentioned this pull request Nov 16, 2023
9 tasks
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.

2 participants