Go: bytes, strings: add ReplaceAll

[HaraldNordgren]

Please answer these questions before submitting your issue. Thanks!

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

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

What did you expect to see?

What did you see instead?

[cznic]

That would be strings.Replace(s, old, new, -1)

[HaraldNordgren]

Thanks for the downvote. I created a PR here https://go-review.googlesource.com/c/go/+/137456 and the implementation is exactly to call strings.Replace(s, old, new, -1).

My problem with the current state of affairs is that I find myself looking up the syntax of this every time I use it. Or if I am reviewing someone else's code I stumble over the -1 because I forget why it's there. So in the spirit of keeping Go simple I want to introduce this helper.

[bradfitz]

Numbers would be interesting here.

What percentage of strings.Replace calls in the wild hard-code -1?

[cznic]

> What percentage of strings.Replace calls in the wild hard-code -1?

I guestimate that on the small sample of my own code it's about 99%. Yet I don't think a function that just sets a default value to one of the parameters of another stdlib function belongs to the stdlib.

[HaraldNordgren]

@bradfitz From my company's codebase, strings.Replace is called 12 times and all but one of them uses -1 as n. That also includes helper functions that are called in lots of places, so the total ratio shifts higher toward almost exclusively the 'ReplaceAll' behavior.

[HaraldNordgren]

@cznic Btw, there are these helper in the standard library already:

from powser2:

func Print(U PS) {
    Printn(U, 1000000000)
}

from draw:

func Draw(dst Image, r image.Rectangle, src image.Image, sp image.Point, op Op) {
    DrawMask(dst, r, src, sp, nil, image.Point{}, op)
}

from ast:

func Inspect(node Node, f func(Node) bool) {
    Walk(inspector(f), node)
}

[magical]

@HaraldNordgren powser2.go is a test, not library code. draw.Draw seems like a good example though.

[bradfitz]

Or strings.Fields and strings.FieldFunc.

Or http.ListenAndServe vs http.Server.ListenAndServe.

Or regexp.Match vs regexp.Compile+Match.

Or regexp.MustCompile vs regexp.Compile, or template.Parse vs template.MustParse.

I don't agree with @cznic thumbs down for this; the standard library is full of convenient-vs-lower-level versions of things.

I'd rather see numbers.

[jimmyfrasche]

in my personal/work code: 16 calls, all -1.

[bradfitz]

In Perkeep, 60 use -1 and 43 use 1. No other values.

[dpinela]

There are 265 calls to strings.Replace in the main Go repo, of which 216 use -1.

[dominikh]

In the Go corpus, excluding vendored code, 1034 of 2213 calls to strings.Replace hard-code -1.

I'm with cznic on this one. Nearly all of the examples provided by bradfitz are more complex (syntactically and semantically) than providing a trivial default value for an argument. Panicking instead of returning an error is a significant change in API. regexp.Match encapsulates two separate operations. strings.Field is far more complex, providing an optimized implementation and pulling in additional dependencies.

In all of these cases, there's really no question as to which function should be used. I would rather not, however, tell people to use ReplaceAll instead of Replace, and I wouldn't want to see a mix of old and new code, either.

[bradfitz]

and I wouldn't want to see a mix of old and new code, either.

That's somewhat inevitable with the standard library's policy of never removing old stuff or mistakes. e.g. we have a mix of Seek values 0 vs os.SEEK_SET vs io.SeekStart and it's not terrible, even if it's not ideal.

[gregory-m]

Excluding generated and vendored code.

In k8s codebase.
100 use -1, 14 use 1. No other values used.

in moby code base:
30 use -1, 6 use 1, No other values used.

[rsc]

regexp.Regexp has a ReplaceAll, so the name is at least with precedent. If we had it to do over again I'd make Replace like Split, where the default is "all" and we could have ReplaceN for a limited count. But we don't have it to do over again. So ReplaceAll seems OK. Should be done to both strings and bytes though.

[cznic]

Replace(s, old, new, -1)
ReplaceAll(s, old, new)

One character saved.

One minute wasted for a Go programmer to lookup the new API function documentation.

Times ~1 million of them.

[dsnet]

FYI. At Google, 65% of all {strings,bytes}.Replace usages hardcode -1.

Edit: looks like I'm late to the party and this is already accepted. Ignore me.
Edit2: s/hardcore/hardcode/... haha.

[bradfitz]

FYI. At Google, 65% of all {strings,bytes}.Replace usages hardcore -1.

That's hardcore.

[gopherbot]

Change https://golang.org/cl/137855 mentions this issue: bytes, strings: add ReplaceAll

[gopherbot]

Change https://golang.org/cl/137856 mentions this issue: all: use strings.ReplaceAll and bytes.ReplaceAll where applicable

[ferhatelmas]

@bradfitz don't you think following encourages bad naming patterns since new is a built-in ?

func Replace(s, old, new string, n int) string

reviewer: don't use built-in as an argument name ?
owner: std lib uses too
reviewer: ...?

[dominikh]

Shadowing built-ins is an explicit feature of the language; nobody will get confused by a variable named new in the context of a replace function.
Also, this isn't the right place for questions regarding style. You can ask those on one of the various forums or mailing lists.

[ferhatelmas]

@dominikh thanks for your insight.

this isn't the right place .... You can ask those on one of the various forums or mailing lists.

Sorry for noise. Will be more careful about it next time.