Skip to content
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: attempt to simplify the multiply operator. #547

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

jayaddison
Copy link

@jayaddison jayaddison commented Mar 7, 2024

  • Reduce the amount of special-case logic required to handle the Ellipsis operator (somewhat equivalent/similar to None values -- translating to: unbound).
  • Extract some parallels between the handling of "zero, one, or more" in the cases of unbounded-maximum or singleton repetitions.

cc @ptmcg

@ptmcg
Copy link
Member

ptmcg commented Mar 8, 2024

Not ignoring you, I have some work to catch up on tomorrow and over the weekend.

return ret
return And([self] * minElements)

return And([self] * minElements) + (Opt(self) * (maxElements - minElements))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No hurry on code review; but from re-inspecting this myself, I'm beginning to wonder about this Opt construction and the effect this could have on the parse output.

In particular: perhaps it's better to have a chain of nested Opt nodes (recursive, instead of iterative as in this PR), because then matching an input against the parse tree can exit as soon as a single token doesn't match -- whereas the And[Opt(...), Opt(...), ...] formulation here would be evaluated in full (I think?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants