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

test: add test that we don't optimize string([]byte(string)) #25834

Closed
bradfitz opened this issue Jun 11, 2018 · 5 comments
Closed

test: add test that we don't optimize string([]byte(string)) #25834

bradfitz opened this issue Jun 11, 2018 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.

Comments

@bradfitz
Copy link
Contributor

From discussion with @golang/proposal-review related to #25805 but mostly unrelated,

We should verify that we have or add a test that string([]byte(s)) doesn't reduce to just s ever. We should test that we always make a copy and return a new string with a new (internal) data pointer.

@rsc mentioned as a use case making a copy of a small part of a large string to prevent the GC from retaining the large string.

/cc @martisch @josharian

@bradfitz bradfitz added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. labels Jun 11, 2018
@josharian
Copy link
Contributor

Echoing @rsc (over in #25805): Given that almost no one will know what string([]byte(s)) does beyond being a no-op at first glance, it seems like a properly-named function would be a lot clearer than a magic compiler non-optimization.

Maybe adding package strings or runtime API instead would be better.

@bradfitz
Copy link
Contributor Author

Except that string([]byte(s)) already exists in the wild for this purpose. We don't want to break that old code.

@martisch
Copy link
Contributor

martisch commented Jun 11, 2018

I agree we can add a test to not accidentally break existing code.

But I also would say that a strings.DeepCopy function or alike (that only allocates once instead of twice) would be nice to replace existing uses of string([]byte(s)) (which afaik the language spec does not mandate needs to allocate) and can be a new faster alternative. New proposal issue?

@bcmills
Copy link
Contributor

bcmills commented Jun 12, 2018

We should verify that we have or add a test that string([]byte(s)) doesn't reduce to just s ever.

Well, to be precise, that should be allowed if (and only if) s is its complete allocation. (That is, it's fine to avoid constructing a new, minimal copy if s is already minimal.)

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/118295 mentions this issue: test: add test to verify that string copies don't get optimized away

dna2github pushed a commit to dna2fork/go that referenced this issue Jun 14, 2018
Fixes golang#25834

Change-Id: I33e58dabfd04b84dfee1a9a3796796b5d19862e7
Reviewed-on: https://go-review.googlesource.com/118295
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Jun 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

5 participants