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
Labels
FeatureRequest FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@minux
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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 as completed Oct 16, 2015
@bradfitz
Copy link
Contributor

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
Copy link

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

@alandonovan
Copy link
Contributor

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
Copy link
Member

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
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 May 23, 2017
@bradfitz bradfitz added FeatureRequest help wanted NeedsFix The path to resolution is known, but the work has not been done. labels May 23, 2017
@kashav
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@odeke-em
Copy link
Member

@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
@ianlancetaylor ianlancetaylor added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. help wanted labels 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
Copy link
Contributor

rsc commented Nov 14, 2018

Fixed by ede5958.

@rsc rsc closed this as completed Nov 14, 2018
changkun added a commit to golang-design/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.
Labels
FeatureRequest FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

10 participants