-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
bpo-40334: Refactor peg_generator to receive a Tokens file when building c code #19745
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
Conversation
6c13937 to
45ca184
Compare
|
This is how the command line looks now with the sub-parsers: Main CLC subparserPython subparser |
1aab4d1 to
d7df6b6
Compare
d7df6b6 to
a371b79
Compare
| if name in self.non_exact_tokens: | ||
| name = name.lower() | ||
| return f"{name}_var", f"_PyPegen_{name}_token(p)" |
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.
I'm wondering if we should special-case NAME here and call _PyPegen_name_token for it and then call _PyPegen_expect_token for all the others (which is what the "named" functions already do anyway). This way we could get rid of these "named" expect functions (e.g. _PyPegen_indent_token).
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.
Hmmm, I think I still like to have the abstraction in case we need to intercede something like with the NAME token in the future (this also allows us to not have "hardcoded" names in the generator) but if you think is better to re-add that in the future and eliminate the wrappers I can totally do it as I don't feel strongly about it.
What do you think?
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.
I think I still like to have the abstraction in case we need to intercede something like with the NAME token in the future
It feels, that that's only the case with STRINGs. I don't really expect to do anything more complicated that just returning the token with all the whitespace ones (INDENT, DEDENT etc.) and I'd re-add those functions for async and await, if it were ever needed.
With that said, I really don't feel strongly about either one, so it's your call! 😃
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.
Ok, I am going to merge this and we can revisit this in further refactors of this code. I have some improvements in mind to avoid checking for sub-strings (like var.startswith("name") when distinguishing the types and we can explore doing this in that PR. 😉
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.
I experimented with your suggestion and the main blocker is that _PyPegen_lookahead is called with these functions and it imposes that they may take the parser as the only argument and there is not a simple way to make _PyPegen_lookahead allow to forward the arguments :(
Edit: I am continuing experimenting with this idea....
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.
I didn't think of this problem. Thanks a lot for doing the investigation!
Co-Authored-By: Lysandros Nikolaou <[email protected]>
Co-Authored-By: Lysandros Nikolaou <[email protected]>
lysnikolaou
left a comment
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.
LGTM! Thanks!
This comment has been minimized.
This comment has been minimized.
Unrelated failure:
|
https://bugs.python.org/issue40334
This PR does the following:
Tokensfile.Tokensfile and add code to parse it and calculate the required token information.