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

Comments

Projects
None yet
6 participants
@bep
Copy link

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

This comment has been minimized.

Copy link
Member

bradfitz commented Feb 17, 2015

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

josharian commented Apr 27, 2015

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

This comment has been minimized.

Copy link
Member

bradfitz commented Apr 27, 2015

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

This comment has been minimized.

Copy link

bep commented Apr 27, 2015

I vote for modifying the input.

@zombiezen

This comment has been minimized.

Copy link
Member

zombiezen commented Jun 4, 2015

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

zombiezen commented Jun 4, 2015

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

This comment has been minimized.

Copy link
Member

zombiezen commented Jun 24, 2015

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

This comment has been minimized.

Copy link
Member

bradfitz commented Jun 24, 2015

This is too late for Go 1.5 at this point.

@bradfitz bradfitz modified the milestones: Go1.6, Go1.5Maybe Jun 24, 2015

@zombiezen

This comment has been minimized.

Copy link
Member

zombiezen commented Jun 24, 2015

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Aug 20, 2015

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

@rsc

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

bradfitz commented Oct 23, 2015

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

Somebody should just maintain a useful version on Github.

@rsc

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

bradfitz commented Nov 26, 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.

@bradfitz bradfitz closed this Nov 26, 2015

@zombiezen

This comment has been minimized.

Copy link
Member

zombiezen commented Nov 30, 2015

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

@rsc

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

bradfitz commented Nov 30, 2015

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

This comment has been minimized.

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.