-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
proposal: maps: add maps.KeysSlice and maps.ValuesSlice #61626
Comments
Does anyone like I'm not sure I'm not bike-shedding, but |
It does not feel necessary. It's (almost) obvious that a map type object does not have its key and values in slices. |
#61900 adds Keys and Values back as iterators. With these, the old maps.Keys(m) would become slices.Collect(maps.Keys(m)) and similarly Values.
becomes
Are there other pattern uses for maps.Keys (other than sorting them) that we should be sure to cover? |
Just my 2¢, but yeah, I feel like as long as slices.Sorted gets added, the uses of maps.KeySlice are infrequent enough that it could just be |
This proposal has been added to the active column of the proposals project |
I suppose creating a set of them using a map[K]struct{}. |
You could do that with an iterator and InsertInto. |
Finishing this proposal discussion is blocked on #61405. |
Change https://go.dev/cl/535215 mentions this issue: |
We were using the size stored in the map, which is the smaller of the real type size and 128. As of CL 61538 we don't use these functions, but we expect to use them again in the future after #61626 is resolved. Change-Id: I7bfb4af5f0e3a56361d4019a8ed7c1ec59ff31fd Reviewed-on: https://go-review.googlesource.com/c/go/+/535215 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> Auto-Submit: Ian Lance Taylor <iant@google.com> Reviewed-by: Keith Randall <khr@google.com>
We were using the size stored in the map, which is the smaller of the real type size and 128. As of CL 61538 we don't use these functions, but we expect to use them again in the future after golang#61626 is resolved. Change-Id: I7bfb4af5f0e3a56361d4019a8ed7c1ec59ff31fd Reviewed-on: https://go-review.googlesource.com/c/go/+/535215 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> Auto-Submit: Ian Lance Taylor <iant@google.com> Reviewed-by: Keith Randall <khr@google.com>
The key issue seems to be #61626 (comment). KeySlices(m) => slices.Collect(maps.Keys(m)) (or slices.Sorted(maps.Keys(m))) and do not need to add these explicitly. |
Based on the discussion above, this proposal seems like a likely decline. |
Need, no. Make devs life easier and Go more approachable yes. |
I feel like the issue is #61900 (comment). I agree it's more elegant to write a single line instead of three, but I don't mind writing those extra lines for the sake of optimization. |
I think it’s a little too soon to permanently decline these. I think we definitely can skip them for now, but it may be that after a few years of experience there turns out to be some reason they are needed after all and this can be reopened then. |
Decline is never permanent. Only accept is permanent. |
No change in consensus, so declined. |
Having to write
to avoid excessive allocations still seems really cumbersome and little approachable to new devs. Given this would be best practice for performance (as @earthboundkid said "those times when you need it"), it couldn't hurt to make this easier to use other than resorting to x/exp/maps. |
Maybe, but why not wait a couple of years and then analyse the Go corpus to determine if it warrants an addition to the standard library.
Every addition to the standard library hurts. The question is whether the benefits outweigh this hurt. |
Imho the argument is above. Writing code that doesn't allocate more than necessary seems worth it imho and hasn't been specifically mentioned before. |
It has been mentioned before: |
Would this be a suitable proposal to be reopened? #68261 does not seem like an attractive alternative. |
Since we need to reserve Keys and Values for iterators.
I think we might need to change the old maps.Keys/Values to maps.KeysSlice/ValuesSlice as mention in 513715
The text was updated successfully, but these errors were encountered: