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.

Sign up for GitHub

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

Jump to bottom

proposal: bytes: add Grow, Clip; maybe add bytes/strings Insert, Delete #48594

Closed
earthboundkid opened this issue Sep 24, 2021 · 10 comments
Closed

proposal: bytes: add Grow, Clip; maybe add bytes/strings Insert, Delete #48594

earthboundkid opened this issue Sep 24, 2021 · 10 comments
Labels
FrozenDueToAge Proposal
Projects
Proposals (old)
Milestone
Proposal

Comments

@earthboundkid
Copy link
Contributor

earthboundkid commented Sep 24, 2021 •
edited

Background:

Accepted proposal #45955 adds a new package called slices with the following API:

package slices

func Equal[T comparable](s1, s2 []T) bool
func EqualFunc[T1, T2 any](s1 []T1, s2 []T2, eq func(T1, T2) bool) bool
func Compare[T constraints.Ordered](s1, s2 []T) int
func CompareFunc[T any](s1, s2 []T, cmp func(T, T) int) int
func Index[T comparable](s []T, v T) int
func IndexFunc[T any](s []T, f func(T) bool) int
func Contains[T comparable](s []T, v T) bool
func Insert[S constraints.Slice[T], T any](s S, i int, v ...T) S
func Delete[S constraints.Slice[T], T any](s S, i, j int) S
func Clone[S constraints.Slice[T], T any](s S) S
func Compact[S constraints.Slice[T], T comparable](s S) S
func CompactFunc[S constraints.Slice[T], T any](s S, cmp func(T, T) bool) S
func Grow[S constraints.Slice[T], T any](s S, n int) S
func Clip[S constraints.Slice[T], T any](s S) S

Of the 14 functions, package bytes already contains 5 identical functions:

package bytes

func Equal(a, b []byte) bool
func Compare(a, b []byte) int
func Index(s, sep []byte) int
func Contains(b, subslice []byte) bool
func Clone(b []byte) []byte

Proposal:

The 4 Func variants don't really make sense for bytes, because they would involve mapping one byte to another, when it's more likely one would want to map one subslice to another. Similarly, Compact seems like a bad fit for bytes, since it would operate on sorted bytes, of which there are only 256.

That leaves 4 functions in slices but not in bytes. Of those 4, Grow and Clip seem extremely useful. I am already using them in my code, and variations of them can be found all over. I propose that they should be added to bytes for convenience and general parity with slices, since users already can find type specific versions of many slices functions in bytes and will reasonably expect to be able to use Grow and Clip as well.

The other two are Insert and Delete. These could also be useful for strings and we might want to add them to both bytes and strings packages. I am more ambivalent about adding these, since their Big O are O(len(s) + len(v)) and O(len(s)-(j-i)), but OTOH, if you know you needed them, why wouldn't you use them?
@gopherbot gopherbot added this to the Proposal milestone Sep 24, 2021
@earthboundkid earthboundkid mentioned this issue Sep 24, 2021
Closed
@seankhliao
Copy link
Member

I don't see why this functionality should be duplicated instead of deprecating/forwarding the existing api to the new slices package

@earthboundkid
Copy link
Contributor Author

earthboundkid commented Sep 24, 2021 •
edited

Why?

  • The bytes package has functionality beyond what is in slices and will never plausibly be deprecated, so it's going to continue to be a thing people use.
  • It's more clear to readers that bytes.Clip(b) is working with bytes than slices.Clip(b).
  • Users who are already using bytes.Equal, etc. may be surprised when they go to use bytes.Grow and find it missing.
  • Minuscule performance and import simplicity benefits.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Sep 24, 2021
@rsc
Copy link
Contributor

rsc commented Oct 6, 2021

It seems like for now we can stick with package slices, which works perfectly fine on []byte.
The bytes package is primarily for UTF-8 interpretations of string data, which means mostly things that don't make sense on generic slices. You did identify a few things that do, but we don't need to add more, at least not without evidence.
On string, Clip and Grow don't make sense, and Insert and Delete can be done with slicing and the + operator.

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 6, 2021
@rsc
Copy link
Contributor

rsc commented Oct 6, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@earthboundkid
Copy link
Contributor Author

The inverse proposal would be to deprecate bytes.Equal and bytes.Compare, and remove bytes.Clone before it ships. I guess that would be fine too, but I don’t think it makes sense to add Clone in 1.18 but not Grow and Clip.

@ianlancetaylor
Copy link
Contributor

The key difference with Clone is that we've historically kept the strings and bytes package as parallel as possible, and strings.Clone can't be replaced by slices.Clone, so since we need strings.Clone we should have bytes.Clone. This argument doesn't apply to Grow and Clip, since strings.Grow and strings.Clip don't make sense.

(The order of bytes.Equal and bytes.Compare went the other way--from the bytes package to the strings package. I agree that it would make sense to deprecate Equal and Compare in both the strings and bytes packages. The only reason they even exist in the strings package is the parallelism between bytes and strings.)

@robpike robpike mentioned this issue Oct 12, 2021
Closed
@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

Deprecate is pretty strong - it's not harming anything to keep the old functions around and working.
But we don't need to add the others.

@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Oct 13, 2021
@earthboundkid
Copy link
Contributor Author

The emoji reaction as of now on the initial post is 2 👍 1 👎 and 1 😕. I'm going to go ahead and close the issue for now because the underlying question is "Do users have an expectation of parity between slices and bytes/strings, as they have an expectation of parity between bytes and strings?" and we just don't know yet. Maybe after slices is released people will ask for bytes.Grow, or maybe people will be so happy with slices.Grow, it won't occur to them to go looking for bytes.Grow. We shall see.

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Oct 20, 2021
@rsc
Copy link
Contributor

rsc commented Oct 20, 2021

This proposal has been declined as retracted.
— rsc for the proposal review group

@golang golang locked and limited conversation to collaborators Oct 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
No open projects
Proposals (old)
Declined
Milestone
Proposal
Development

No branches or pull requests
5 participants
@rsc @earthboundkid @ianlancetaylor @gopherbot @seankhliao