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

Add left recursion support for rules without args #266

Merged

Conversation

fgasperij
Copy link
Contributor

@fgasperij fgasperij commented Jul 19, 2021

Adding a straightforward implementation of the packrat memoization to support left recursion in rules without args.

These changes would resolve #262.

Any feedback is welcome, let me know what you think!

@fgasperij fgasperij force-pushed the add-left-recursion-support branch 2 times, most recently from 55d0e8c to 68198a7 Compare July 23, 2021 17:04
Copy link
Owner

@kevinmehall kevinmehall left a comment

Choose a reason for hiding this comment

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

Nice!

I'm thinking this should be a different attribute, maybe #[cache_left_rec] or #[cache(recursive)], rather than changing the behavior of all #[cache]. The overhead of this for a rule that does not use recursion is that it will parse the rule a second time, and always find the same end position and exit the loop. I'm not sure that double-parsing is acceptable, given than #[cache] is most likely applied to rules that have nontrivial parse time when you want to avoid repeatedly parsing them.

peg-macros/translate.rs Outdated Show resolved Hide resolved
peg-macros/analysis.rs Outdated Show resolved Hide resolved
peg-macros/translate.rs Outdated Show resolved Hide resolved
@fgasperij fgasperij force-pushed the add-left-recursion-support branch 2 times, most recently from bb79ba4 to f3fe977 Compare July 26, 2021 07:33
@fgasperij
Copy link
Contributor Author

Yes, completely agree, making all #[cache] users pay that extra call makes no sense, now it's #[cache_left_rec]. Should we change the left recursion error message to say something like If you want to use left recursion, add #[cache_left_rec], but bear in mind that it will be a little more costly.

peg-macros/analysis.rs Outdated Show resolved Hide resolved
@kevinmehall
Copy link
Owner

Should we change the left recursion error message

I'm not sure the error message is the place to go into a ton of detail about the tradeoffs, but there should probably be a section in the documentation describing the different cache modes, and maybe a comparison of the approaches for parsing nested expressions. I'm happy to write this if you'd rather just get this merged.

@fgasperij
Copy link
Contributor Author

Yes, if we can merge this one, and add the doc in a different PR that would be great. Let me give it a go, I'll put up a PR tomorrow.

@kevinmehall kevinmehall merged commit 4b146b4 into kevinmehall:master Jul 28, 2021
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.

Add support for left-recursive rules
2 participants