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 combinator parsing to parser.rs #106

Merged
merged 6 commits into from
Feb 22, 2020

Conversation

halfbro
Copy link

@halfbro halfbro commented Oct 23, 2019

Relates to #12
This PR adds combinator-based parsing using nom.

This allows the code to uniformly handle numerics (percentages, decimals, integers), and can also eventually provide error output in case of parser failure (not implemented yet).

I haven't had a chance to benchmark the differences between the parsers yet.

@halfbro halfbro force-pushed the combinator-parsing branch from be4870f to ea49234 Compare October 23, 2019 02:12
@sharkdp
Copy link
Owner

sharkdp commented Oct 29, 2019

Thank you very much for your contribution.

This is a bit hard to review, since you also refactored the test cases (which is probably a good thing, haven't looked at it in detail). I am assuming that you did not delete or alter any test cases?

@sharkdp
Copy link
Owner

sharkdp commented Jan 1, 2020

Any update on this?

@sharkdp
Copy link
Owner

sharkdp commented Feb 22, 2020

I re-instated the removed tests (I don't really understand why they were removed).

I also added a simple benchmark which shows that this new implementation is significantly faster 👍

image

@sharkdp sharkdp merged commit 4807607 into sharkdp:master Feb 22, 2020
@sharkdp
Copy link
Owner

sharkdp commented Feb 22, 2020

@halfbro Thank you very much!

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.

2 participants