gh-124889: Remove redundant artificial rules in PEG parser #124893
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Cache in C PEG-generator reworked: we save artificial rules in cache by Node string representation as a key instead of Node object itself. As a result total count of artificial rules in parsers.c is lowered from 283 to 170. More natural number ordering is used for the names of artificial rules.
| self.cache[node] = self.generate_call(node.alts[0].items[0]) | ||
| return self.generate_call(node.alts[0].items[0]) | ||
|
|
||
| node_str = f"{node}" |
There was a problem hiding this comment.
It would be great if we can abstract this away in a function. Maybe
def generate_artificial_rule(node, prefix, new_rule_fn):
node_str = f"{node}"
key = f"{prefix}_{node_str}"
if key in self.cache:
name = self.cache[key]
else:
name = new_rule_fn()
self.cache[key] = name
return name
|
Excellent find @efimov-mikhail 👌 In general LGTM, I just have a small suggestion and we can land it. I am skipping news since this is not a user visible change. |
…ython#124893) Auxiliary method CCallMakerVisitor._generate_artificial_rule_call is added. Its purpose is abstracting work with artificial rules cache.
731ea7f to
0a8f197
Compare
|
What do you think about this refactoring? I've moved visit_Rhs method to emphasize similarity of visit_* methods with artificial rules. Name "_generate_artificial_rule_call" of method seems to to be suitable, since we already have "generate_call" method in this class. |
There was a problem hiding this comment.
Thanks for this @efimov-mikhail! It's a very nice improvement. I've left a couple of more minor comments about the code.
python#124893) Explicit using of "is_repeat1" kwarg is added to visit_Repeat0 and visit_Repeat1 methods. Its slightly improve code readabitily.
|
And that's it! Fantastic work @efimov-mikhail! Thanks a lot for the catch and the PR. Looking forward for more contributions 🙂 |
|
Great thanks! Collaboration was perfect! |
Cache in C PEG-generator reworked:
we save artificial rules in cache by Node string representation as a key instead of Node object itself. As a result total count of artificial rules in parsers.c is lowered from 283 to 170. More natural number ordering is used for the names of artificial rules.