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

adjust reg compile code location #27

Merged
merged 2 commits into from Jun 21, 2022

Conversation

clamyang
Copy link
Contributor

@clamyang clamyang commented May 3, 2022

1.adjust reg compile code location
2.pre alloc regs slice cap
3.fix typo

2.pre alloc regs slice cap
3.fix typo

return func(pass *analysis.Pass) (interface{}, error) {
// Precompile the regexps, report the error
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside of having the compilation steps here is that this anonymous function will be executed once per package. Having it within the body of run executes it once on startup. Now, this could completely be a useless optimisation, but I don't have any numbers on hand to make a strong argument either way. Hence, doing the compilation only once steers on the safer side.

Do you have any reasoning for shifting it down here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello, @tomarrell !

I'm very excited you can reply me.

I agree with you, this anonymous function will be executed in per package. My original intention is fix the error check.

I had an illusion after seeing testdata dir. i thought every package has a .wrapcheck.yaml, in general, one project only have one .wrapcheck.yaml.

so, what do you think error check in the follow:

func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) {
	// Precompile the regexps, report the error
	var (
		ignoreSigRegexp        []*regexp.Regexp
		ignoreInterfaceRegexps []*regexp.Regexp
		ignorePackageGlobs     []glob.Glob
		err                    error
	)

	ignoreSigRegexp, err = compileRegexps(cfg.IgnoreSigRegexps)
	if err != nil {
		log.Fatalf("compile IgnoreSigRegexps failed: %v", err)
	}

	ignoreInterfaceRegexps, err = compileRegexps(cfg.IgnoreInterfaceRegexps)
	if err != nil {
		log.Fatalf("compile IgnoreInterfaceRegexps failed: %v", err)
	}
        // ...
}

Copy link
Owner

@tomarrell tomarrell May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the late reply, I've had a work event and some holiday the past week.

Unfortunately, calling log.Fatal will cause unintended consequences for golangci-lint. So unfortunately we need to leave it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @tomarrell Thanks for the reply.

I have already revert the change of reg compile code location.I can't find a new way that is more better to fix the error check.

Thanks.


return func(pass *analysis.Pass) (interface{}, error) {
// Precompile the regexps, report the error
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside of having the compilation steps here is that this anonymous function will be executed once per package. Having it within the body of run executes it once on startup. Now, this could completely be a useless optimisation, but I don't have any numbers on hand to make a strong argument either way. Hence, doing the compilation only once steers on the safer side.

Do you have any reasoning for shifting it down here?

@tomarrell
Copy link
Owner

Thanks for the contribution! Just one minor question before I can move things forward.

1 similar comment
@tomarrell
Copy link
Owner

Thanks for the contribution! Just one minor question before I can move things forward.

@tomarrell tomarrell merged commit a9d3b53 into tomarrell:master Jun 21, 2022
@clamyang clamyang deleted the patch-from-bqyang branch June 22, 2022 01:17
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