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
strings: ToLower not returning a copy for strings already in lower case #30987
Comments
Nice catch. Although the commit you pointed to is for the ToUpper one. My vote is to document this behavior. We don't need to unnecessarily copy the string when it's not required. Even Map has a fast path that returns the original string instead of a copy. https://github.com/golang/go/blob/master/src/strings/strings.go#L497 |
At the language level, there's no difference between a string and a copy of that string. They may retain different amounts of memory for their backing store, but memory usage is not a language-level concept. So I don' think it warrants changing the docs for That said, we understand that it's occasionally necessary to copy strings to free a large backing store. We've been musing about a way to do that. I think we probably need a runtime function which guarantees a new backing store (or knows that the current one is small enough). Currently, |
Makes sense. But the OP has a point. If the docs say that a function returns a copy, but it actually doesn't in some cases, the the docs are technically incorrect.
Agree, but this needs to be documented somewhere as a general pointer. Perhaps in the package overview or somewhere else. |
Maybe the docs of |
Yes, I think that's the case. We should remove "a copy" from the strings version, as it doesn't mean anything. |
Change https://golang.org/cl/171735 mentions this issue: |
What version of Go are you using (
go version
)?1.12.1
Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?linux/amd64
What did you do?
Using
strings.ToLower
on strings in upper or lower case:https://play.golang.org/p/eyPCCj_zCtF
What did you expect to see?
As per the documentation (https://github.com/golang/go/blob/master/src/strings/strings.go#L583), have the same behaviour for both cases: getting a new copy of the string
What did you see instead?
Strings with only lower case do not get copied, the original string is kept
Comments
This seems to be a result of an optimisation, 13cfb15, and in particular the fully optimized path of lower case ASCII: https://github.com/golang/go/blob/master/src/strings/strings.go#L597
As string are immutable, it doesn't seem to be a huge issue (except that the documentation is wrong), but as far as I understand this can result in pseudo memory leaks if one relies (as per the documentation) on ToLower to do a copy of small slices of larger strings (and only keeping references to the lower case slice)
The text was updated successfully, but these errors were encountered: