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

[C++] Optimize LL1Analyzer #3522

Merged
merged 1 commit into from
Feb 1, 2022
Merged

Conversation

jcking
Copy link
Collaborator

@jcking jcking commented Jan 31, 2022

No description provided.

@jcking
Copy link
Collaborator Author

jcking commented Feb 1, 2022

This optimizes LL1Analyzer by removing extra heap allocations, removing usage of shared_ptr where possible, and reducing stack usage by reducing the number of arguments passed on the stack recursively.

@mike-lischke

@jcking jcking marked this pull request as ready for review February 1, 2022 03:59
@jcking
Copy link
Collaborator Author

jcking commented Feb 1, 2022

I also got rid of the finally usage because that's really only useful for exceptions and there is no code catching exceptions from LL1Analyzer, so a throw will kill th state anyway. I also used LL1AnalyzerImpl to avoid changing the LL1Analyzer contract, but I'm happy to modify LL1Analyzer directly if you prefer.

Copy link
Member

@mike-lischke mike-lischke left a comment

Choose a reason for hiding this comment

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

That's tricky to review, as a large block was moved around and I cannot compare both. Also I don't see a real benefit of moving a part of the implementation into an inner class, but it cleans things up a bit, so I'm willing to approve, but pls check my comment yet.

@jcking jcking force-pushed the cpp-optimize-ll1analyzer branch from f20db32 to d992adf Compare February 1, 2022 14:10
@jcking jcking requested a review from mike-lischke February 1, 2022 14:11
Copy link
Member

@mike-lischke mike-lischke left a comment

Choose a reason for hiding this comment

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

@parrt This patch can be merged.

@parrt parrt added this to the 4.10 milestone Feb 1, 2022
@parrt parrt merged commit 79ec55f into antlr:master Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants