-
Notifications
You must be signed in to change notification settings - Fork 22
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
refactor: Move grammar parser constructor to codegen_grammar #746
refactor: Move grammar parser constructor to codegen_grammar #746
Conversation
This is an overdue cleanup; rather than shipping the constructor with the language definition, it should be a part of the crate responsible for the codegen/parser logic itself.
|
@@ -3,8 +3,6 @@ pub use solidity::SolidityDefinition; | |||
codegen_language_macros::compile!(Language( | |||
name = Solidity, | |||
root_item = SourceUnit, | |||
// TODO(#638): For now this is on par with the DSL v1 definition to minimize the fallout. |
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.
We decided against virtual EndOfInput
tokens, so not bringing that trivia parser back from the original DSLv2. Also, the trivia has to require something, i.e. we shouldn't create empty rules. Will be unnested anyway to sibling tokens with #737
The new definition was also wrong, as in, we need to find an ergomonic way to accept MultilineComment
as part of the leading trivia but only if it doesn't have newline in it; I'd consider this a separate work item (new "feature") and it's not of urgent priority - we should probably stick to single-line comments and revisit this at some point.
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.
Also, the trivia has to require something, i.e. we shouldn't create empty rules
Not sure I'm following. Since they are siblings, we should be using the original ZeroOrMore
, as it should always be possible to have no leading trivia, right?
We will run that parser (that adds all its results to the "current" parent rule node), and possibly adding nothing.
-> Same concern for the removed TODO comment in model/terminals/trivia.rs
accept MultilineComment as part of the leading trivia but only if it doesn't have newline in it
Not sure I'm following either? newlines is legal/valid in leading trivia.
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.
Left one concern about the removed TODO comments, which I don't think are related. If you don't mind, let's keep them as-is until we discuss this further.
Otherwise, LGTM.
Replaced by #747 |
Follow-up to #650
Nothing ground-breaking, just cleaning up the language definition crate and moving the grammar construction to be a part of the crate responsible for the codegen/parser logic itself.