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.

Sign up for GitHub

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

Jump to bottom

cmd/vet: vet should recommend to use operators instead of strings.Compare to compare strings #50209

Closed
zigo101 opened this issue Dec 16, 2021 · 6 comments
Closed

cmd/vet: vet should recommend to use operators instead of strings.Compare to compare strings #50209

zigo101 opened this issue Dec 16, 2021 · 6 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@zigo101
Copy link

zigo101 commented Dec 16, 2021 •
edited
Loading

What version of Go are you using (go version)?

$ go version
go version go1.17.5 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

import "strings"

var x = "abcdefgx"
var y = "abcdefgy"

func main() {
	if strings.Compare(x, y) == 0 {
		println(1)
	}
}
$ go vet main.go

What did you expect to see?

warning that strings.Compare(x, y) == 0 is slower than x == y for some cases.

By the current strings.Compare implementation, all calls to this functions should be warned. (sorry, there are still some cases the strings.Compare way is more performant)

What did you see instead?

Nothing outputted.
@zigo101
Copy link
Author

zigo101 commented Dec 16, 2021

related: #50167

@cherrymui cherrymui added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 16, 2021
@cherrymui cherrymui added this to the Backlog milestone Dec 16, 2021
@cherrymui
Copy link
Member

I think vet is more for correctness, not for optimizations. Also, as you mentioned, there are valid uses of strings.Compare. I'm not sure vet should do this.

cc @timothy-king

@timothy-king
Copy link
Contributor

Here are the vet inclusion criteria https://github.com/golang/go/blob/master/src/cmd/vet/README

This suggestion plausibly fails the "correctness" criteria:

Correctness:

Vet's checks are about correctness, not style. A vet check must identify real or
potential bugs that could cause incorrect compilation or execution. A check that
only identifies stylistic points or alternative correct approaches to a situation
is not acceptable.

An argument might be that this is a not a style suggestion and is instead an efficiency suggestion. My personal inclination is that this qualifies as 'identifying an alternative correct approach to a situation'. I am curious how others feel about this though.

Other thoughts:

  • This does not mean this is not a good recommendation for a different tool than cmd/vet.
  • This suggestion is somewhat consistent with the comment in compare.go

@ianlancetaylor
Copy link
Member

I agree that this is not a suitable check for cmd/vet.

@davecheney
Copy link
Contributor

This seems like an ideal candidate for a tool like gocritic

@zigo101 zigo101 mentioned this issue Dec 17, 2021
Closed
@zigo101
Copy link
Author

zigo101 commented Dec 17, 2021

OK, I created an issue in the go-critic project: go-critic/go-critic#1167

@zigo101 zigo101 closed this as completed Dec 17, 2021
@golang golang locked and limited conversation to collaborators Dec 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@davecheney @timothy-king @ianlancetaylor @gopherbot @cherrymui @zigo101