strings: maybe strings.Compare should be optimized

[zigo101]

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?

https://sourcegraph.com/search?q=context:global+switch+strings.Compare+lang:Go+&patternType=literal

What did you expect to see?

The strings.Compare should do one comparison for all cases.

What did you see instead?

The strings.Compare does two comparisons for some cases.

https://github.com/golang/go/blob/go1.17.5/src/strings/compare.go#L13

[seankhliao]

The comment in the code explains why not

[zigo101]

I'm aware of that comment. But I think it should be optimized, as many projects use the function often.

[zigo101]

@seankhliao please reopen this issue.

[seankhliao]

Many projects writing poor code doesn't mean we should optimize it, especially when we're actively trying to discourage it.

[zigo101]

Such code in many projects is not poor at all.

[zigo101]

If you think they are poor, please show the not-poor way.

[ianlancetaylor]

The comment explicitly says that we don't want to encourage people to use strings.Compare. That hasn't changed. We expect code to compare strings using operators like == and <. Those are the cases that we should optimize (in the compiler), not the strings.Compare function. (And if we do optimize those cases in the compiler, then perhaps strings.Compare will be optimized too as a side-effect.)

[zigo101]

I understand this. But there are situations in which strings.Compare should be used and the strings.Compare is expected to be implemented performantly.

In other words, I don't think the comment is reasonable.

bytes.Compare has made the optimization, so I think it is easy for strings.Compare too.

More specifically, there are many uses cases shown in https://sourcegraph.com/search?q=context:global+switch+strings.Compare+lang:Go+&patternType=literal are like:

func foo(x, y string) {
	switch strings.Compare(x, y) {
	case -1:
		// do something 1
	case 0:
		// do something 2
	case 1:
		// do something 3
	}
}

I think this is a common use case.

[ianlancetaylor]

Instead of worrying about strings.Compare, let's make this efficient. If we do that strings.Compare will become more efficient as a side-effect.

func foo(x, y string) {
    switch {
    case x < y:
        // do something 1
    case x == y:
        // do something 2
    default:
        // do something 3
    }
}

[zigo101]

So for some cases, even if the operator way is not efficient, it is still recommended to use this way, by making the other way also inefficient deliberately?

[ianlancetaylor]

I would say, instead, focus on making the more readable approach efficient. Don't focus on making the less readable approach efficient. We aren't making strings.Compare deliberately inefficient; we're making it the same as the more readable approach.

[zigo101]

Readability might be a subjective word. Personally, I don't think there is a readability difference between the two ways.

The two ways are not overlapped fully. There are situations in which using strings.Compare (assumed it is implemented normally as expected) is more efficient, and there are also situations in which using operators is more efficient. I really don't understand why to deliberately prefer one to the other.

[zigo101]

And personally, the role of recommending the operator way to the strings.Compare way for some situations is go vet, instead of the standard packages.

[zigo101]

And the third, the current docs of the strings.Compare doesn't mentioned the current implementation is slower than expected for some situations.

[zigo101]

One advantage of the strings.Compare way is the case order has much less impact on the performance, but the case order in the operator way impacts the performance much.

[zigo101]

I will close this issue, for I found a similar one: #31187

But I think close the issue for using strings.Compare is poor code or less-readable code is not acceptable.

[zigo101]

OK, it is closed already.

[zigo101]

Some notes:

• the strings.Compare function is used in toolchain code: https://github.com/golang/go/blob/go1.19/src/cmd/go/internal/modindex/read.go#L822
• and this function is recommended by sort.Find: https://github.com/golang/go/blob/go1.19/src/sort/search.go#L88-L99

Some conflict with the internal comment of strings.Compare: Basically no one should use strings.Compare.

BTW, there are more uses of this function out of switch blocks: https://sourcegraph.com/search?q=context:global+strings.Compare+lang:Go&patternType=standard