-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
gh-90928: Improve static initialization of keywords tuple in AC #95907
Conversation
Deduce num_keywords from f.parameters
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.
Thanks for working on this! Mostly LGTM.
Also, can you drop "num_keywords" from template_dict
now?
When you're done making the requested changes, leave the comment: |
It is already dropped :) |
I have made the requested changes; please review again. |
Thanks for making the requested changes! @ericsnowcurrently: please review the changes made to this pull request. |
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
I'll leave it up to you about the two comments I left.
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Thanks for reviewing; highly appreciated. I'll make a quick adjustment before landing. |
- inline len(keywords) - use NUM_KEYWORDS constant for improved readability in generated code
I'm done with the last round of adjustments. I ended up adding both of your last suggestions. Thanks again. |
FYI, I'll wait with merging until Kumar has finished his review. |
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!
Thanks for the reviews! |
Calculate
num_keywords
during template generation instead of leaving it to the preprocessor. This makes for much smaller and easier to read (FWIW) AC output.See discussion in #95860 (comment)