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

Simplify binop parsing #273

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Simplify binop parsing #273

merged 1 commit into from
Dec 6, 2023

Conversation

irevoire
Copy link
Contributor

@irevoire irevoire commented Dec 4, 2023

Hey, just a little PR that tries to simplify adding new binop operations.

It's not as worth it as I thought though, so feel free to close it if you think it actually makes the code harder to read.

Having two closures in the parse_binop method is a bit hard to use IMO, but overall, being able to use the same method for 7 other parsers is interesting.

@sharkdp
Copy link
Owner

sharkdp commented Dec 5, 2023

I had the same thought when I reviewed your logical-operator PR — nice! If there are no performance regressions, I'm happy to accept this. I think it's definitely an improvement.

@irevoire
Copy link
Contributor Author

irevoire commented Dec 5, 2023

On my side, I didn't measure any performance regression:

Import prelude          time:   [16.242 ms 16.293 ms 16.345 ms]
                        change: [-0.3828% +0.1170% +0.5889%] (p = 0.63 > 0.05)
                        No change in performance detected.

@sharkdp sharkdp merged commit 10ab90a into sharkdp:master Dec 6, 2023
14 checks passed
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