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 MapIter.SetKey, SetValue, and friends #46131

Closed
rsc opened this issue May 12, 2021 · 9 comments
Closed

reflect: add MapIter.SetKey, SetValue, and friends #46131

rsc opened this issue May 12, 2021 · 9 comments

Comments

@rsc
Copy link
Contributor

rsc commented May 12, 2021

Over at #32424 (comment), @randall77 suggests that a performance problem with map iteration can be fixed by adding MapIter.SetKey and MapIter.SetValue methods.

It's possible others should be changed but this is the most significant from a performance standpoint.

@rsc rsc changed the title reflect: add MapIter.SetKey, SetValue, and friends proposal: reflect: add MapIter.SetKey, SetValue, and friends May 12, 2021
@gopherbot gopherbot added this to the Proposal milestone May 12, 2021
@rsc
Copy link
Contributor Author

rsc commented May 12, 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

josharian added a commit to tailscale/go that referenced this issue May 18, 2021
These augment the existing MapIter.Key and MapIter.Value methods.
The existing methods return new Values.
Constructing these new Values often requires allocating.
These methods allow the caller to bring their own storage.

The naming is somewhat unfortunate, in that the spec
uses the word "element" instead of "value",
as do the reflect.Type methods.
In a vacuum, MapIter.SetElem would be preferable.
However, matching the existing methods is more important.

Fixes golang#32424
Fixes golang#46131

Change-Id: I19c4d95c432f63dfe52cde96d2125abd021f24fa
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/320929 mentions this issue: reflect: add MapIter.SetKey and MapIter.SetValue

josharian added a commit to tailscale/go that referenced this issue May 18, 2021
These augment the existing MapIter.Key and MapIter.Value methods.
The existing methods return new Values.
Constructing these new Values often requires allocating.
These methods allow the caller to bring their own storage.

The naming is somewhat unfortunate, in that the spec
uses the word "element" instead of "value",
as do the reflect.Type methods.
In a vacuum, MapIter.SetElem would be preferable.
However, matching the existing methods is more important.

Fixes golang#32424
Fixes golang#46131

Change-Id: I19c4d95c432f63dfe52cde96d2125abd021f24fa
josharian added a commit to tailscale/go that referenced this issue May 18, 2021
These augment the existing MapIter.Key and MapIter.Value methods.
The existing methods return new Values.
Constructing these new Values often requires allocating.
These methods allow the caller to bring their own storage.

The naming is somewhat unfortunate, in that the spec
uses the word "element" instead of "value",
as do the reflect.Type methods.
In a vacuum, MapIter.SetElem would be preferable.
However, matching the existing methods is more important.

Fixes golang#32424
Fixes golang#46131

Change-Id: I19c4d95c432f63dfe52cde96d2125abd021f24fa
@rsc
Copy link
Contributor Author

rsc commented May 19, 2021

/cc @randall77; any reason not to do this?

@randall77
Copy link
Contributor

No, sounds fine to me.

@rsc
Copy link
Contributor Author

rsc commented May 26, 2021

Does anyone object to adding these?

@rsc
Copy link
Contributor Author

rsc commented Jun 2, 2021

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

@rsc
Copy link
Contributor Author

rsc commented Jun 9, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: reflect: add MapIter.SetKey, SetValue, and friends reflect: add MapIter.SetKey, SetValue, and friends Jun 9, 2021
@rsc rsc modified the milestones: Proposal, Backlog Jun 9, 2021
@dsnet
Copy link
Member

dsnet commented Aug 30, 2021

Minor bikeshed, I was a little confused by the direction of setting. In reflect, we have:

func (v Value) Set(x Value)
func (v Value) SetBool(x bool)
func (v Value) SetBytes(x []byte)
func (v Value) SetCap(n int)
func (v Value) SetComplex(x complex128)
func (v Value) SetFloat(x float64)
func (v Value) SetInt(x int64)
func (v Value) SetLen(n int)
func (v Value) SetMapIndex(key, elem Value)
func (v Value) SetPointer(x unsafe.Pointer)
func (v Value) SetString(x string)
func (v Value) SetUint(x uint64)

All of theses take the input argument and store it into the receiver, v.

Now we added:

func (it *MapIter) SetKey(dst Value)
func (it *MapIter) SetValue(dst Value)

However, contrary to reflect.Value.SetXXX, it is state in the receiver that it being store into the input argument. It is the opposite direction.

Perhaps we should rename it as SetKeyInto and SetValueInto?

@josharian
Copy link
Contributor

Perhaps we should rename it as SetKeyInto and SetValueInto?

Or StoreKey and StoreValue?

@rsc rsc moved this to Accepted in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@golang golang locked and limited conversation to collaborators Aug 30, 2022
@rsc rsc removed this from Proposals Oct 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants