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

bytes: add Replacer #9905

Closed
bep opened this issue Feb 17, 2015 · 20 comments
Closed

bytes: add Replacer #9905

bep opened this issue Feb 17, 2015 · 20 comments

Comments

@bep
Copy link
Contributor

bep commented Feb 17, 2015

The strings.Replacer is a real race car.

It would be very useful to have a bytes.Replacer with similar feature set.

@bradfitz
Copy link
Contributor

If we do this, we should consider whether we can write it once with type T, and use go generate to spit out the strings and bytes versions. I don't know if that'll be more or less work.

@rsc
Copy link
Contributor

rsc commented Apr 10, 2015

Yes, we should add Replacer. I do not believe we should use go generate, though. Generated code is harder to work with and debug, and this will need debugging. It's not like a String method.

@rsc rsc added this to the Go1.5 milestone Apr 10, 2015
@josharian
Copy link
Contributor

I want this too. One immediate use would be reducing allocs in the linker.

An API question: Should Replacer.Replace modify the input (a la append) or always return a copy? The former could avoid allocs in cases in which the length does not grow or the input has sufficient capacity, but it means callers have to be more careful.

@bradfitz
Copy link
Contributor

I would say it should be allowed to re-use the passed-in memory. Otherwise there's little advantage over using strings.Replacer instead. That said, almost everything in bytes already is documented as returning a copy, which is a little sad.

@bep
Copy link
Contributor Author

bep commented Apr 27, 2015

I vote for modifying the input.

@zombiezen
Copy link
Contributor

Couldn't the strings.Replacer implementation use the bytes.Replacer implementation? There's a potential slowdown for WriteString, but it might make it simpler.

(Also, has the window for implementing this in Go 1.5 passed? I'd like to take a shot at it.)

@bradfitz
Copy link
Contributor

bradfitz commented Jun 4, 2015

No, I wouldn't write the strings version in terms of the bytes version. One deals with runes and one deals with bytes, in addition to the major string/[]byte representation difference.

This is still tagged Go 1.5, so perhaps if you do it soon it can still go in.

@zombiezen
Copy link
Contributor

Makes sense, but after reading through the code, it appears that strings doesn't operate on runes. The representation difference is still a good argument for keeping them distinct.

@rsc rsc modified the milestones: Go1.5Maybe, Go1.5 Jun 8, 2015
@zombiezen
Copy link
Contributor

This is coming along; I'm hoping to have a CL ready this week. One question on API: should the arguments of bytes.NewReplacer be strings or byte slices? strings seem easier to work with (and other functions in the bytes package use strings for character sets), but I can also see the argument for byte slices.

@bradfitz
Copy link
Contributor

This is too late for Go 1.5 at this point.

@bradfitz bradfitz modified the milestones: Go1.6, Go1.5Maybe Jun 24, 2015
@zombiezen
Copy link
Contributor

Drat. I'll keep a package going at https://github.com/zombiezen/go-bytesreplacer as I work on the implementation.

@gopherbot
Copy link

CL https://golang.org/cl/13797 mentions this issue.

@rsc
Copy link
Contributor

rsc commented Oct 23, 2015

If Replacer is added it needs to mimic strings.Replacer. In particular it needs to return a copy of the input, just like bytes.Replace and bytes.Map do. Obviously one can run faster by rewriting the input slice, but this one function shouldn't be different from the design of the rest of the package.

@bradfitz
Copy link
Contributor

Then it's useless and we should just close this bug.

Somebody should just maintain a useful version on Github.

@rsc
Copy link
Contributor

rsc commented Nov 26, 2015

FWIW, I disagree that it's useless.

@rsc rsc modified the milestones: Unplanned, Go1.6 Nov 26, 2015
@bradfitz
Copy link
Contributor

I say it's useless because if you didn't mind copies, you could just use strings.NewReplacer and do the []byte(string) and string([]byte) copies yourself before/after strings.NewReplacer. Having bytes.NewReplacer that just does those conversions for you (but with the added huge negative of all that mostly-duplicated code) buys you very little.

@zombiezen
Copy link
Contributor

For those interested, I just submitted this as go4.org/bytereplacer. Thanks @bradfitz!

@rsc
Copy link
Contributor

rsc commented Nov 30, 2015

I say it's useless because if you didn't mind copies, you could just use strings.NewReplacer and do the []byte(string) and string([]byte) copies yourself before/after strings.NewReplacer. Having bytes.NewReplacer that just does those conversions for you (but with the added huge negative of all that mostly-duplicated code) buys you very little.

By that argument we should delete the entire bytes package.

@bradfitz
Copy link
Contributor

By that argument we should delete the entire bytes package.

No, by that argument, we should delete only some of the bytes package. (if only we could!)

Functions in package bytes returning bools or ints such as Compare, Contains, Count, Equal, EqualFold, HasPrefix, HasSuffix, Index, IndexAny, IndexBytes, IndexFunc, etc are fine. The mostly-useless ones are the ones returning []byte copies.

We have to keep them for compatibility, but there's no need to add more.

@rsc
Copy link
Contributor

rsc commented Nov 30, 2015

We'll just have to agree to disagree.

@golang golang locked and limited conversation to collaborators Dec 1, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants