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

Replace Cases with enum Tag and switch statements #99

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Apanatshka
Copy link
Contributor

I had a look at the Cases pattern in the statix.solver project and tried replacing it with switch statements. This seems to work out alright, doesn't gives much more ugly code (some cases I'd argue the code even gets better), and should be somewhat faster I think. Can you try running your benchmark/profiling example on this version and see if it makes a difference in performance? This doesn't get rid of all the lambdas, but it should help I think/hope.

@Apanatshka
Copy link
Contributor Author

Welp, the tests fail, so I probably made a mistake in refactoring one of these things. I'll ping when this passes the tests.

@Apanatshka
Copy link
Contributor Author

Cool, the tests run now.

@Apanatshka Apanatshka requested a review from AZWN April 28, 2022 10:53
@Apanatshka
Copy link
Contributor Author

@AZWN we can look at this together tomorrow if you like

@AZWN
Copy link
Contributor

AZWN commented Apr 28, 2022

Looks good, and sounds like a plan! Tomorrow after lunch?

@Apanatshka
Copy link
Contributor Author

Works for me. If you can find the time, please run a benchmark / profiling example beforehand so we can evaluate the performance implications as well as the code changes.

@Apanatshka
Copy link
Contributor Author

So the good news is I was able to fairly easily change to using switch for ITerm for you, which should help to make your profiling info a bit more straightforward to read. The bad news is it still does nothing for performance. Still, I hope it helps you a bit with profiling at least...

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