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

R parser improvements #4208

Closed

Conversation

TwoOfTwelve
Copy link
Contributor

In its current state the R grammar is hard to use, since there is one parser rule containing almost everything. Additionally, most tokens are defined implicitly. Therefore the AST elements that are generated can only be distinguished by comparing the strings in the tokens. That requires a lot of hard-coded string literals in the application code.

To make using the AST easier, this PR defines most tokens explicitly and breaks up the parser rules into smaller ones. I tried to do that as general as possible, but I focused on what is needed for JPlag.
With these changes the AST nodes can be distinguished by their types. Different cases can be checked via the names of the tokens instead of using a string comparison.
Terminal nodes can be identified through named constants.

As far as I can tell the functionality of the grammar has not changed. I would appreciate it if someone could confirm that.

This PR is a follow up to #4160.

@kaby76
Copy link
Contributor

kaby76 commented Aug 14, 2024

The rewrite of expr will likely impact the performance. You should check this. Have you considered the alternative that uses alt labeling?

@TwoOfTwelve
Copy link
Contributor Author

I wasn't aware of that feature. It looks perfect for what I need. I'll see if I can make it work using labels, thank you.

@KvanTTT KvanTTT added the R label Aug 15, 2024
@TwoOfTwelve
Copy link
Contributor Author

I've created a new PR using the suggested solution (#4215).

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

Successfully merging this pull request may close these issues.

3 participants