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

Memoize Cop#rspec_pattern #965

Closed
wants to merge 1 commit into from
Closed

Conversation

marcandre
Copy link
Contributor

Currently #2 in the memory allocated when running RuboCop... And #1 should disappear soon too 😅

See detailed list in whitequark/parser#721

@bquorning
Copy link
Collaborator

If we are going to memoize anyway, we might as well revert the previous optimisation done in #507 ?

@marcandre
Copy link
Contributor Author

marcandre commented Jul 13, 2020

If we are going to memoize anyway, we might as well revert the previous optimisation done in #507 ?

Good question.

I just went for the quickest fix tbh, I didn't delve into the code and I didn't actually even check the impact. The memoization is only per cop though, that code might still execute more than it could?

@bquorning
Copy link
Collaborator

I just went for the quickest fix tbh

Would you prefer that I take over the PR? I’d like to dig a bit more into how we can perform this calculation only once across all cops.

@marcandre
Copy link
Contributor Author

With pleasure. I was running my tests with bin/rubocop-profile --memory lib/rubocop/cop/mixin, but you should get similar results for other combinations I imagine.

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