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

reflect: add a range like interface for Value #11104

Closed
minux opened this issue Jun 6, 2015 · 11 comments
Closed

reflect: add a range like interface for Value #11104

minux opened this issue Jun 6, 2015 · 11 comments

Comments

@minux
Copy link
Member

@minux minux commented Jun 6, 2015

The current reflect.Value interface for traversing maps requires allocating
all the keys at once (reflect.Value.MapKeys), which is not efficient for
huge maps.

Additionally, as floating point NaNs are never equal to each other, the
current interface makes it impossible to get the value for NaN keys.

We can introduce this

func (v Value) MapKeyValues() [][2]Value

to solve the NaN problem, but I'm more interested in the first problem.

Actually, if we decide to introduce a range like operation on reflect.Value,
I think we should make it work for other range-able types (channels,
arrays, strings and slices) too.

@minux minux added this to the Go1.6 milestone Jun 6, 2015
@gordonklaus

This comment has been minimized.

Copy link
Contributor

@gordonklaus gordonklaus commented Sep 21, 2015

reflect.Value already allows efficient emulation of range semantics for arrays, slices, strings, and channels, so a fully generic range-like operation is unnecessary. It need only work for maps.

There's an argument for making it generic for consistency's sake, but I think it's pretty weak. For arrays, slices, and channels, range is only syntactic sugar, which doesn't provide the same value in reflect. Even if there were a generic range operation, I would still favor the existing Len/Index methods for ranging arrays and slices and the Recv method for ranging a channel.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 16, 2015

I'm not particularly worried, especially about NaNs. It doesn't seem important enough to introduce yet more API here.

@rsc rsc closed this Oct 16, 2015
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Oct 16, 2015

This is a little sad. I was hoping there would be an efficient way to use reflect to range over a map.

@golang golang locked and limited conversation to collaborators Oct 17, 2016
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 23, 2016

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

@alandonovan

This comment has been minimized.

Copy link
Contributor

@alandonovan alandonovan commented Nov 23, 2016

Another reason for this feature is that there's no way to iterate over a map while respecting deletions that occur during the iteration, as documented by the spec, making it impossible to implement a Go interpreter in Go using maps to represent maps.

I've quickly sketched an implementation in https://go-review.googlesource.com/33572.

@alandonovan alandonovan reopened this Nov 23, 2016
@ALTree ALTree modified the milestones: Go1.9, Go1.6 Jan 11, 2017
@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Jan 11, 2017

There's something wrong with this issue: it's 1) open 2) assigned to a closed milestone (go1.6).

I moved it to go1.9 and unlocked it.

@golang golang unlocked this conversation Jan 11, 2017
@ALTree ALTree removed the FrozenDueToAge label Jan 11, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 May 23, 2017
@kashav

This comment has been minimized.

Copy link
Contributor

@kashav kashav commented Jul 7, 2017

Would love to give this a go (found via https://dev.golang.org/imfeelinghelpful btw), any particular reason https://go-review.googlesource.com/33572 is marked as DO NOT SUBMIT (is it just incomplete or was it only meant to be a PoC)?

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Jul 7, 2017

@kshvmdn, looks like the CL is marked for Go 1.10 and this issue are marked for Go 1.10.

@alandonovan, what's the state of the CL?

@alandonovan

This comment has been minimized.

Copy link
Contributor

@alandonovan alandonovan commented Jul 8, 2017

The CL works to the best of my knowledge but has not been reviewed by someone who knows the runtime.

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Nov 23, 2017

@bradfitz moved the CL to Go1.11 and so will I punt this issue to Go1.11 too, thanks for the CL @alandonovan and others for the discussions.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 27, 2018
gopherbot pushed a commit that referenced this issue Aug 22, 2018
Example of use:

	iter := reflect.ValueOf(m).MapRange()
 	for iter.Next() {
		k := iter.Key()
		v := iter.Value()
		...
	}

See issue #11104

Q. Are there any benchmarks that would exercise the new calls to
   copyval in existing code?

Change-Id: Ic469fcab5f1d9d853e76225f89bde01ee1d36e7a
Reviewed-on: https://go-review.googlesource.com/33572
Reviewed-by: Keith Randall <khr@golang.org>
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 14, 2018

Fixed by ede5958.

@rsc rsc closed this Nov 14, 2018
changkun added a commit to changkun/go-under-the-hood that referenced this issue Jan 27, 2019
@golang golang locked and limited conversation to collaborators Nov 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
10 participants
You can’t perform that action at this time.