[go] reflect: rename Mapiter.SetKey to Value.SetIterKey
40 views
Skip to first unread message
Keith Randall (Gerrit)
Oct 14, 2021, 8:47:41 PM10/14/21
to Joe Tsai, Josh Bleecher Snyder, goph...@pubsubhelper.golang.org, Keith Randall, golang-co...@googlegroups.com
Attention is currently required from: Joe Tsai, Josh Bleecher Snyder.
Keith Randall would like Joe Tsai and Josh Bleecher Snyder to review this change.
reflect: rename Mapiter.SetKey to Value.SetIterKey
Same for Value.
Add a bigger test. Include some shouldPanic checks.
Fix a bug in assignment conversion.
Fixes #48294
Change-Id: Id863ee5122a5787a7b35574b18586fd24d118788
---
M src/reflect/value.go
M src/reflect/all_test.go
2 files changed, 112 insertions(+), 16 deletions(-)
diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go
index 5e10cc7..858e82e 100644
--- a/src/reflect/all_test.go
+++ b/src/reflect/all_test.go
@@ -15,6 +15,7 @@
"math"
"math/rand"
"os"
+ "reflect"
. "reflect"
"reflect/internal/example1"
"reflect/internal/example2"
@@ -348,8 +349,8 @@
iter := v.MapRange()
for iter.Next() {
- iter.SetKey(k)
- iter.SetValue(e)
+ k.SetIterKey(iter)
+ e.SetIterValue(iter)
want := m[k.String()]
got := e.Interface()
if got != want {
@@ -366,8 +367,8 @@
got := int(testing.AllocsPerRun(10, func() {
iter := v.MapRange()
for iter.Next() {
- iter.SetKey(k)
- iter.SetValue(e)
+ k.SetIterKey(iter)
+ e.SetIterValue(iter)
}
}))
// Making a *MapIter allocates. This should be the only allocation.
@@ -7475,9 +7476,9 @@
var seenk, seenv uint64
iter.Reset(ValueOf(m3))
for iter.Next() {
- iter.SetKey(kv)
+ kv.SetIterKey(iter)
seenk ^= kv.Uint()
- iter.SetValue(kv)
+ kv.SetIterValue(iter)
seenv ^= kv.Uint()
}
if seenk != 0b111 {
@@ -7619,3 +7620,81 @@
t.Fatalf("(%s).ConvertibleTo(%s) = true, want false", t3, t4)
}
}
+
+func TestSetIter(t *testing.T) {
+ data := map[string]int{
+ "foo": 1,
+ "bar": 2,
+ "baz": 3,
+ }
+
+ m := ValueOf(data)
+ i := m.MapRange()
+ k := New(TypeOf("")).Elem()
+ v := New(TypeOf(0)).Elem()
+ shouldPanic("Value.SetIterKey called before Next", func() {
+ k.SetIterKey(i)
+ })
+ shouldPanic("Value.SetIterValue called before Next", func() {
+ k.SetIterValue(i)
+ })
+ data2 := map[string]int{}
+ for i.Next() {
+ k.SetIterKey(i)
+ v.SetIterValue(i)
+ data2[k.Interface().(string)] = v.Interface().(int)
+ }
+ if !DeepEqual(data, data2) {
+ t.Errorf("maps not equal, got %v want %v", data2, data)
+ }
+ shouldPanic("Value.SetIterKey called on exhausted iterator", func() {
+ k.SetIterKey(i)
+ })
+ shouldPanic("Value.SetIterValue called on exhausted iterator", func() {
+ k.SetIterValue(i)
+ })
+
+ i.Reset(m)
+ i.Next()
+ shouldPanic("Value.SetIterKey using unaddressable value", func() {
+ ValueOf("").SetIterKey(i)
+ })
+ shouldPanic("Value.SetIterValue using unaddressable value", func() {
+ ValueOf(0).SetIterValue(i)
+ })
+ shouldPanic("value of type string is not assignable to type int", func() {
+ New(TypeOf(0)).Elem().SetIterKey(i)
+ })
+ shouldPanic("value of type int is not assignable to type string", func() {
+ New(TypeOf("")).Elem().SetIterValue(i)
+ })
+
+ // Make sure assignment conversion works.
+ var x interface{}
+ y := reflect.ValueOf(&x).Elem()
+ y.SetIterKey(i)
+ if _, ok := data[x.(string)]; !ok {
+ t.Errorf("got key %s which is not in map", x)
+ }
+ y.SetIterValue(i)
+ if x.(int) < 1 || x.(int) > 3 {
+ t.Errorf("got value %d which is not in map", x)
+ }
+
+ // Try some key/value types which are direct interfaces.
+ a := 88
+ b := 99
+ pp := map[*int]*int{
+ &a: &b,
+ }
+ i = ValueOf(pp).MapRange()
+ i.Next()
+ y.SetIterKey(i)
+ if got := *y.Interface().(*int); got != a {
+ t.Errorf("pointer incorrect: got %d want %d", got, a)
+ }
+ y.SetIterValue(i)
+ if got := *y.Interface().(*int); got != b {
+ t.Errorf("pointer incorrect: got %d want %d", got, b)
+ }
+}
diff --git a/src/reflect/value.go b/src/reflect/value.go
index 39b82a8..afcb1c4 100644
--- a/src/reflect/value.go
+++ b/src/reflect/value.go
@@ -1651,16 +1651,16 @@
return copyVal(ktype, iter.m.flag.ro()|flag(ktype.Kind()), iterkey)
}
-// SetKey assigns dst to the key of iter's current map entry.
+// SetIterKey assigns to dst the key of iter's current map entry.
// It is equivalent to dst.Set(i.Key()), but it avoids allocating a new Value.
// As in Go, the key must be assignable to dst's type.
-func (iter *MapIter) SetKey(dst Value) {
+func (dst Value) SetIterKey(iter *MapIter) {
if !iter.hiter.initialized() {
- panic("MapIter.SetKey called before Next")
+ panic("reflect: Value.SetIterKey called before Next")
}
iterkey := mapiterkey(&iter.hiter)
if iterkey == nil {
- panic("MapIter.SetKey called on exhausted iterator")
+ panic("reflect: Value.SetIterKey called on exhausted iterator")
}
dst.mustBeAssignable()
@@ -1672,7 +1672,7 @@
t := (*mapType)(unsafe.Pointer(iter.m.typ))
ktype := t.key
- key := Value{ktype, iterkey, iter.m.flag | flag(ktype.Kind())}
+ key := Value{ktype, iterkey, iter.m.flag | flag(ktype.Kind()) | flagIndir}
key = key.assignTo("reflect.MapIter.SetKey", dst.typ, target)
typedmemmove(dst.typ, dst.ptr, key.ptr)
}
@@ -1692,16 +1692,16 @@
return copyVal(vtype, iter.m.flag.ro()|flag(vtype.Kind()), iterelem)
}
-// SetValue assigns dst to the value of iter's current map entry.
+// SetIterValue assigns to dst the value of iter's current map entry.
// It is equivalent to dst.Set(i.Value()), but it avoids allocating a new Value.
// As in Go, the value must be assignable to dst's type.
-func (iter *MapIter) SetValue(dst Value) {
+func (dst Value) SetIterValue(iter *MapIter) {
if !iter.hiter.initialized() {
- panic("MapIter.SetValue called before Next")
+ panic("reflect: Value.SetIterValue called before Next")
}
iterelem := mapiterelem(&iter.hiter)
if iterelem == nil {
- panic("MapIter.SetValue called on exhausted iterator")
+ panic("reflect: Value.SetIterValue called on exhausted iterator")
}
dst.mustBeAssignable()
@@ -1713,7 +1713,7 @@
t := (*mapType)(unsafe.Pointer(iter.m.typ))
vtype := t.elem
- elem := Value{vtype, iterelem, iter.m.flag | flag(vtype.Kind())}
+ elem := Value{vtype, iterelem, iter.m.flag | flag(vtype.Kind()) | flagIndir}
elem = elem.assignTo("reflect.MapIter.SetValue", dst.typ, target)
typedmemmove(dst.typ, dst.ptr, elem.ptr)
}
To view, visit change 356049. To unsubscribe, or for help writing mail filters, visit settings.
Josh Bleecher Snyder (Gerrit)
Oct 14, 2021, 9:05:49 PM10/14/21
to Keith Randall, goph...@pubsubhelper.golang.org, Josh Bleecher Snyder, Go Bot, Joe Tsai, golang-co...@googlegroups.com
Attention is currently required from: Joe Tsai, Keith Randall.
Patch set 1:Code-Review +2Trust +1
4 comments:
File src/reflect/all_test.go:
Patch Set #1, Line 18:
"reflect"reflect is dot-imported on the next line. (this trips me up every time too. maybe we should remove the dot import.)
Patch Set #1, Line 7639:
k.SetIterValue(i)v.SetIterValue
I assume we have type mismatch panic tests elsewhere.
Patch Set #1, Line 7654:
k.SetIterValue(i)v
File src/reflect/value.go:
Patch Set #1, Line 1675:
key := Value{ktype, iterkey, iter.m.flag | flag(ktype.Kind()) | flagIndir}thanks. figuring out value flags is in my experience the single hardest part about working on package reflect. if anyone felt inspired during the upcoming freeze to expand the docs about flags, that’d be really useful.
To view, visit change 356049. To unsubscribe, or for help writing mail filters, visit settings.
Joe Tsai (Gerrit)
Oct 14, 2021, 9:09:08 PM10/14/21
to Keith Randall, goph...@pubsubhelper.golang.org, Joe Tsai, Josh Bleecher Snyder, Go Bot, Joe Tsai, golang-co...@googlegroups.com
Attention is currently required from: Joe Tsai, Keith Randall.
4 comments:
File src/reflect/value.go:
iter
s/dst/v/g
to be consistent with other Value methods
iter
s/dst/v/g
to be consistent with other Value methods
To view, visit change 356049. To unsubscribe, or for help writing mail filters, visit settings.
Keith Randall (Gerrit)
Oct 14, 2021, 10:14:07 PM10/14/21
to Keith Randall, goph...@pubsubhelper.golang.org, Joe Tsai, Josh Bleecher Snyder, Go Bot, Joe Tsai, golang-co...@googlegroups.com
Attention is currently required from: Joe Tsai.
8 comments:
File src/reflect/all_test.go:
Patch Set #1, Line 18:
"reflect"
reflect is dot-imported on the next line. (this trips me up every time too. […]
Done
Patch Set #1, Line 7639:
k.SetIterValue(i)
v.SetIterValue […]
Yes, they are below (line 7665 - 7670).
Patch Set #1, Line 7654:
k.SetIterValue(i)v
Done
File src/reflect/value.go:
iter
Done
s/dst/v/g […]
Done
Patch Set #1, Line 1675:
key := Value{ktype, iterkey, iter.m.flag | flag(ktype.Kind()) | flagIndir}
thanks. […]
Ack
iter
Done
s/dst/v/g […]
Done
To view, visit change 356049. To unsubscribe, or for help writing mail filters, visit settings.
Keith Randall (Gerrit)
Oct 14, 2021, 10:14:14 PM10/14/21
to Keith Randall, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Joe Tsai.
Keith Randall uploaded patch set #2 to this change.
reflect: rename Mapiter.SetKey to Value.SetIterKey
Same for Value.
Add a bigger test. Include some shouldPanic checks.
Fix a bug in assignment conversion.
Fixes #48294
Change-Id: Id863ee5122a5787a7b35574b18586fd24d118788
---
M src/reflect/value.go
M src/reflect/all_test.go
2 files changed, 125 insertions(+), 30 deletions(-)
To view, visit change 356049. To unsubscribe, or for help writing mail filters, visit settings.
Keith Randall (Gerrit)
Oct 14, 2021, 11:49:21 PM10/14/21
to Keith Randall, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Go Bot, Joe Tsai, Josh Bleecher Snyder, Joe Tsai, golang-co...@googlegroups.com
Keith Randall submitted this change.
1 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/reflect/all_test.go
Insertions: 1, Deletions: 2.
@@ -15,7 +15,6 @@
"math"
"math/rand"
"os"
- "reflect"
. "reflect"
"reflect/internal/example1"
"reflect/internal/example2"
@@ -7671,7 +7670,7 @@
// Make sure assignment conversion works.
var x interface{}
- y := reflect.ValueOf(&x).Elem()
+ y := ValueOf(&x).Elem()
y.SetIterKey(i)
if _, ok := data[x.(string)]; !ok {
t.Errorf("got key %s which is not in map", x)
```
```
The name of the file: src/reflect/value.go
Insertions: 18, Deletions: 18.
@@ -1651,10 +1651,10 @@
return copyVal(ktype, iter.m.flag.ro()|flag(ktype.Kind()), iterkey)
}
-// SetIterKey assigns to dst the key of iter's current map entry.
-// It is equivalent to dst.Set(i.Key()), but it avoids allocating a new Value.
-// As in Go, the key must be assignable to dst's type.
-func (dst Value) SetIterKey(iter *MapIter) {
+// SetIterKey assigns to v the key of iter's current map entry.
+// It is equivalent to v.Set(iter.Key()), but it avoids allocating a new Value.
+// As in Go, the key must be assignable to v's type.
+func (v Value) SetIterKey(iter *MapIter) {
if !iter.hiter.initialized() {
panic("reflect: Value.SetIterKey called before Next")
}
@@ -1663,18 +1663,18 @@
panic("reflect: Value.SetIterKey called on exhausted iterator")
}
- dst.mustBeAssignable()
+ v.mustBeAssignable()
var target unsafe.Pointer
- if dst.kind() == Interface {
- target = dst.ptr
+ if v.kind() == Interface {
+ target = v.ptr
}
t := (*mapType)(unsafe.Pointer(iter.m.typ))
ktype := t.key
key := Value{ktype, iterkey, iter.m.flag | flag(ktype.Kind()) | flagIndir}
- key = key.assignTo("reflect.MapIter.SetKey", dst.typ, target)
- typedmemmove(dst.typ, dst.ptr, key.ptr)
+ key = key.assignTo("reflect.MapIter.SetKey", v.typ, target)
+ typedmemmove(v.typ, v.ptr, key.ptr)
}
// Value returns the value of iter's current map entry.
@@ -1692,10 +1692,10 @@
return copyVal(vtype, iter.m.flag.ro()|flag(vtype.Kind()), iterelem)
}
-// SetIterValue assigns to dst the value of iter's current map entry.
-// It is equivalent to dst.Set(i.Value()), but it avoids allocating a new Value.
-// As in Go, the value must be assignable to dst's type.
-func (dst Value) SetIterValue(iter *MapIter) {
+// SetIterValue assigns to v the value of iter's current map entry.
+// It is equivalent to v.Set(iter.Value()), but it avoids allocating a new Value.
+// As in Go, the value must be assignable to v's type.
+func (v Value) SetIterValue(iter *MapIter) {
if !iter.hiter.initialized() {
panic("reflect: Value.SetIterValue called before Next")
}
@@ -1704,18 +1704,18 @@
panic("reflect: Value.SetIterValue called on exhausted iterator")
}
- dst.mustBeAssignable()
+ v.mustBeAssignable()
var target unsafe.Pointer
- if dst.kind() == Interface {
- target = dst.ptr
+ if v.kind() == Interface {
+ target = v.ptr
}
t := (*mapType)(unsafe.Pointer(iter.m.typ))
vtype := t.elem
elem := Value{vtype, iterelem, iter.m.flag | flag(vtype.Kind()) | flagIndir}
- elem = elem.assignTo("reflect.MapIter.SetValue", dst.typ, target)
- typedmemmove(dst.typ, dst.ptr, elem.ptr)
+ elem = elem.assignTo("reflect.MapIter.SetValue", v.typ, target)
+ typedmemmove(v.typ, v.ptr, elem.ptr)
}
// Next advances the map iterator and reports whether there is another
```
Approvals:
Josh Bleecher Snyder: Looks good to me, approved; Trusted
Keith Randall: Trusted; Run TryBots
Go Bot: TryBots succeeded
reflect: rename Mapiter.SetKey to Value.SetIterKey
Same for Value.
Add a bigger test. Include some shouldPanic checks.
Fix a bug in assignment conversion.
Fixes #48294
Change-Id: Id863ee5122a5787a7b35574b18586fd24d118788
Reviewed-on: https://go-review.googlesource.com/c/go/+/356049
Trust: Keith Randall <k...@golang.org>
Trust: Josh Bleecher Snyder <josh...@gmail.com>
Run-TryBot: Keith Randall <k...@golang.org>
TryBot-Result: Go Bot <go...@golang.org>
Reviewed-by: Josh Bleecher Snyder <josh...@gmail.com>
---
M src/reflect/value.go
M src/reflect/all_test.go
2 files changed, 131 insertions(+), 30 deletions(-)
diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go
index 5e10cc7..58156e0 100644
--- a/src/reflect/all_test.go
+++ b/src/reflect/all_test.go
@@ -348,8 +348,8 @@
iter := v.MapRange()
for iter.Next() {
- iter.SetKey(k)
- iter.SetValue(e)
+ k.SetIterKey(iter)
+ e.SetIterValue(iter)
want := m[k.String()]
got := e.Interface()
if got != want {
@@ -366,8 +366,8 @@
got := int(testing.AllocsPerRun(10, func() {
iter := v.MapRange()
for iter.Next() {
- iter.SetKey(k)
- iter.SetValue(e)
+ k.SetIterKey(iter)
+ e.SetIterValue(iter)
}
}))
// Making a *MapIter allocates. This should be the only allocation.
@@ -7475,9 +7475,9 @@
var seenk, seenv uint64
iter.Reset(ValueOf(m3))
for iter.Next() {
- iter.SetKey(kv)
+ kv.SetIterKey(iter)
seenk ^= kv.Uint()
- iter.SetValue(kv)
+ kv.SetIterValue(iter)
seenv ^= kv.Uint()
}
if seenk != 0b111 {
@@ -7619,3 +7619,81 @@+ y := ValueOf(&x).Elem()index 39b82a8..abcc346 100644
--- a/src/reflect/value.go
+++ b/src/reflect/value.go
@@ -1651,30 +1651,30 @@
return copyVal(ktype, iter.m.flag.ro()|flag(ktype.Kind()), iterkey)
}
-// SetKey assigns dst to the key of iter's current map entry.
-// It is equivalent to dst.Set(i.Key()), but it avoids allocating a new Value.
-// As in Go, the key must be assignable to dst's type.
-func (iter *MapIter) SetKey(dst Value) {
+// SetIterKey assigns to v the key of iter's current map entry.
+// It is equivalent to v.Set(iter.Key()), but it avoids allocating a new Value.
+// As in Go, the key must be assignable to v's type.
+func (v Value) SetIterKey(iter *MapIter) {
if !iter.hiter.initialized() {
- panic("MapIter.SetKey called before Next")
+ panic("reflect: Value.SetIterKey called before Next")
}
iterkey := mapiterkey(&iter.hiter)
if iterkey == nil {
- panic("MapIter.SetKey called on exhausted iterator")
+ panic("reflect: Value.SetIterKey called on exhausted iterator")
}
- dst.mustBeAssignable()
+ v.mustBeAssignable()
var target unsafe.Pointer
- if dst.kind() == Interface {
- target = dst.ptr
+ if v.kind() == Interface {
+ target = v.ptr
}
t := (*mapType)(unsafe.Pointer(iter.m.typ))
ktype := t.key
- key := Value{ktype, iterkey, iter.m.flag | flag(ktype.Kind())}
- key = key.assignTo("reflect.MapIter.SetKey", dst.typ, target)
- typedmemmove(dst.typ, dst.ptr, key.ptr)
+ key := Value{ktype, iterkey, iter.m.flag | flag(ktype.Kind()) | flagIndir}
+ key = key.assignTo("reflect.MapIter.SetKey", v.typ, target)
+ typedmemmove(v.typ, v.ptr, key.ptr)
}
// Value returns the value of iter's current map entry.
@@ -1692,30 +1692,30 @@
return copyVal(vtype, iter.m.flag.ro()|flag(vtype.Kind()), iterelem)
}
-// SetValue assigns dst to the value of iter's current map entry.
-// It is equivalent to dst.Set(i.Value()), but it avoids allocating a new Value.
-// As in Go, the value must be assignable to dst's type.
-func (iter *MapIter) SetValue(dst Value) {
+// SetIterValue assigns to v the value of iter's current map entry.
+// It is equivalent to v.Set(iter.Value()), but it avoids allocating a new Value.
+// As in Go, the value must be assignable to v's type.
+func (v Value) SetIterValue(iter *MapIter) {
if !iter.hiter.initialized() {
- panic("MapIter.SetValue called before Next")
+ panic("reflect: Value.SetIterValue called before Next")
}
iterelem := mapiterelem(&iter.hiter)
if iterelem == nil {
- panic("MapIter.SetValue called on exhausted iterator")
+ panic("reflect: Value.SetIterValue called on exhausted iterator")
}
- dst.mustBeAssignable()
+ v.mustBeAssignable()
var target unsafe.Pointer
- if dst.kind() == Interface {
- target = dst.ptr
+ if v.kind() == Interface {
+ target = v.ptr
}
t := (*mapType)(unsafe.Pointer(iter.m.typ))
vtype := t.elem
- elem := Value{vtype, iterelem, iter.m.flag | flag(vtype.Kind())}
- elem = elem.assignTo("reflect.MapIter.SetValue", dst.typ, target)
- typedmemmove(dst.typ, dst.ptr, elem.ptr)
+ elem := Value{vtype, iterelem, iter.m.flag | flag(vtype.Kind()) | flagIndir}
+ elem = elem.assignTo("reflect.MapIter.SetValue", v.typ, target)
+ typedmemmove(v.typ, v.ptr, elem.ptr)
}
// Next advances the map iterator and reports whether there is another
To view, visit change 356049. To unsubscribe, or for help writing mail filters, visit settings.
Josh Bleecher Snyder (Gerrit)
Oct 15, 2021, 1:54:17 AM10/15/21
to Keith Randall, goph...@pubsubhelper.golang.org, Go Bot, Joe Tsai, Josh Bleecher Snyder, Joe Tsai, golang-co...@googlegroups.com
1 comment:
File src/reflect/all_test.go:
Patch Set #1, Line 7654:
k.SetIterValue(i)Done
neither this nor the other s/k/v/ fix above made it into the final patchset. want to send a follow-up?
To view, visit change 356049. To unsubscribe, or for help writing mail filters, visit settings.
Keith Randall (Gerrit)
Oct 15, 2021, 2:03:33 AM10/15/21
to Keith Randall, goph...@pubsubhelper.golang.org, Go Bot, Joe Tsai, Josh Bleecher Snyder, Joe Tsai, golang-co...@googlegroups.com
1 comment:
File src/reflect/all_test.go:
Patch Set #1, Line 7654:
k.SetIterValue(i)
neither this nor the other s/k/v/ fix above made it into the final patchset. […]
Sorry about that, not sure how I reverted that after I made the fix.
To view, visit change 356049. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages