reflect: add Value.Clear; support anytype->interface{}, Slice->(*)Array in Value.Convert, fix Copy
#4543
Conversation
ben-krieger
force-pushed
the
reflect-clear-and-more
branch
from
1abc718
to
ba79b94
Compare
ben-krieger
changed the title
Value.Clear; support anytype->interface{}, Slice->(*)Array in Value.Convert
|
Thanks for the thorough review @dgryski! I'll spend some time this weekend fixing it up and adding more tests. |
ben-krieger
force-pushed
the
reflect-clear-and-more
branch
3 times, most recently
from
192b1ff
to
4adcc42
Compare
|
|
Got it. So it seems that arrays in some cases have become addrable and settable, when they shouldn't. Let me see if I can figure out why... |
|
It's almost certainly the flag changes for |
ben-krieger
force-pushed
the
reflect-clear-and-more
branch
2 times, most recently
from
e391914
to
6340647
Compare
Tried RO, but couldn't get it working, so I reverted the change that broke it. (I was also uncomfortable with that code, because I didn't understand the underlying mechanism that distinguished between arrays > 64 bits in size.) However, the test I added for Any thoughts on the proper way to fix #4554? |
ben-krieger
force-pushed
the
reflect-clear-and-more
branch
2 times, most recently
from
1e3d12c
to
97c6c38
Compare
ben-krieger
changed the title
Value.Clear; support anytype->interface{}, Slice->(*)Array in Value.ConvertValue.Clear; support anytype->interface{}, Slice->(*)Array in Value.Convert, fix Copy
@dgryski I think this is ready for (hopefully the last) review. All tests are passing now and the fix for #4554 matches a lot of other code within the reflect package that check either indirect OR size > uintptr. I don't see any more low-hanging fruit for test coverage, but let me know what you think. Thanks! |
|
One quick comment. I'll do a full review tomorrow (Monday) and then hopefully we can get this merged. Thanks for all your work on this! |
ben-krieger
force-pushed
the
reflect-clear-and-more
branch
4 times, most recently
from
1401cad
to
a80c83b
Compare
| @@ -995,6 +1001,10 @@ func (t *rawType) AssignableTo(u Type) bool { | |||
| return true | |||
| } | |||
|
|
|||
| if t.underlying() == u.(*rawType).underlying() && (!t.isNamed() || !u.(*rawType).isNamed()) { | |||
There was a problem hiding this comment.
See https://go.dev/ref/spec#Assignability
If the underlying types are the same, only one can be named.
ben-krieger
force-pushed
the
reflect-clear-and-more
branch
from
a80c83b
to
ae03d47
Compare
|
@aykevl Any comments? |
|
Took a look, looks good to me. |
Fixes #4554
This PR adds a few things to the reflect package.
Value.Clearinterface{}, i.e.reflect.ValueOf(42).Convert(reflect.TypeFor[interface{}]())reflect.ValueOf([]int{1, 2, 3}).Convert(reflect.TypeFor[[3]int]())reflect.Copywhen the source is an arrayThe motivation was to allow compiling the CBOR library at https://github.com/fido-device-onboard/go-fdo/tree/tinygo-server-support/cbor. However, this PR can be split into smaller ones if desired.