Demand Ply 3.11 -- SIMICS-23703 - #454
Conversation
| @wraps(rule) | ||
| def wrapped(t): | ||
| fixup_emptyprod_lexpos(t) | ||
| return rule(t) |
There was a problem hiding this comment.
You were performance stingy, so I kept the way fixup_emptyprod_lexpos(t) gets called identical to before this change. That said, back when I actually measured the impact of the len(t) == 1 branch being taken a gazillion times, it was minimal, so I'd like to just unify these two into a simple if len(t) <= 1: check. If you're fine with it.
... Honestly I'd prefer to skip out on the emptyprod_re check entirely and unconditionally augment if len(t) <= 1: but I'd get if that'd be a step too far for you.
There was a problem hiding this comment.
What you have definitely looks good enough to accept without benchmarking.
If you want to compress stuff further, then I suppose squashing if/else to allow inlining fixup_emptyprod_lexpos into its only remaining use would also be uncontroversial.
| def wrapper(rule): | ||
| rule.__doc__ = 'ident : ' + "\n| ".join(idents) | ||
| return rule | ||
| return wrapper |
There was a problem hiding this comment.
lex.TOKEN has changed operation since 3.4, and sets .regex instead of __doc__. As you could've guessed, .regex is parsed as, well, a regex. Hence this change.
| assert lines[0:3] == ['', 'Conflicts:', ''], lines | ||
| conflicts = lines[3:] | ||
| assert len(conflicts) == 10, conflicts | ||
| assert len(conflicts) == (11 if version == '12' else 13), conflicts |
There was a problem hiding this comment.
I haven't looked into these yet, but I'm willing to bet this is just a case of Ply having gotten better at detecting conflicts. At the very least, t126 passes.
There was a problem hiding this comment.
I think these are scary and need to be looked into -- in particular because it shows a diff between DML 1.2 and 1.4 that wasn't seen before.
Ply 3.11 actually offers an escape hatch to let decorated production rules work
db4d2c6 to
fb92698
Compare
2 participants
t126 passes locally with a custom venv featuring Ply 3.11.
This will have to be merged after a base-deps PR with both the bump to the Ply version and the bump to the DMLC submodule.