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

optimize deprecatedComment_checker: delete regexp usage #1188

Merged
merged 3 commits into from Jan 6, 2022

Conversation

peakle
Copy link
Contributor

@peakle peakle commented Jan 6, 2022

pr for: #1183
benchmarks in progress

@quasilyte
Copy link
Member

We may need more tests to make sure that nothing is broken after this optimization. Could you add some, just to be sure?

@peakle
Copy link
Contributor Author

peakle commented Jan 6, 2022

We may need more tests to make sure that nothing is broken after this optimization. Could you add some, just to be sure?

added test cases

@quasilyte
Copy link
Member

quasilyte commented Jan 6, 2022

Benchmarks are not necessary if you can measure that gocritic becomes faster after these changes.
The code looks good so far.

Maybe https://github.com/sharkdp/hyperfine can help here.

@quasilyte quasilyte merged commit 8a81465 into go-critic:master Jan 6, 2022
@peakle
Copy link
Contributor Author

peakle commented Jan 6, 2022

Benchmarks are not necessary if you can measure that gocritic becomes faster after these changes. The code looks good so far.

Maybe https://github.com/sharkdp/hyperfine can help here.

i executed old and new version on https://github.com/kubernetes/kubernetes and as i see call graph looks better

but also i found that now it impossible to parse multiline comments like this one:

// ComponentStatusList represents the list of component statuses
// Deprecated: This API is deprecated in v1.19+
type ComponentStatusList struct {}

will fix it in new pr :)

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.

None yet

2 participants