code review 4423043: changes for more restricted reflect.SetValue

changes for more restricted reflect.SetValue

[rsc, 2011-04-18 16:21:38]

Hello golang-dev (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-04-18 18:19:35]

http://codereview.appspot.com/4423043/diff/2002/src/pkg/gob/decode.go
File src/pkg/gob/decode.go (right):

http://codereview.appspot.com/4423043/diff/2002/src/pkg/gob/decode.go#newcode...
src/pkg/gob/decode.go:1279: func allocValue(t reflect.Type) reflect.Value {
i wonder if this should be in reflect.

http://codereview.appspot.com/4423043/diff/2002/src/pkg/gob/decoder.go
File src/pkg/gob/decoder.go (right):

http://codereview.appspot.com/4423043/diff/2002/src/pkg/gob/decoder.go#newcod...
src/pkg/gob/decoder.go:178: // value will be discarded.  XXX
XXX?

http://codereview.appspot.com/4423043/diff/2002/src/pkg/gob/decoder.go#newcod...
src/pkg/gob/decoder.go:184: panic("gob: DecodeValue of unassignable value")
should this panic? just return an error.

[rsc, 2011-04-18 18:23:41]

>
http://codereview.appspot.com/4423043/diff/2002/src/pkg/gob/decode.go#newcode...
> src/pkg/gob/decode.go:1279: func allocValue(t reflect.Type)
> reflect.Value {
> i wonder if this should be in reflect.

I wonder too.  I was very tempted to make reflect.New(t) return an
addressable, assignable t instead of a *t.  In the end I decided to
be consistent with Go.  If we change the name we could do it though.
AllocValue?

I also wondered whether NewValue should be named Valueof,
analogous to Typeof.

>
http://codereview.appspot.com/4423043/diff/2002/src/pkg/gob/decoder.go#newcod...
> src/pkg/gob/decoder.go:178: // value will be discarded.  XXX
> XXX?

Updated:

// DecodeValue reads the next value from the connection.
// If v is the zero reflect.Value (v.Kind() == Invalid), DecodeValue
discards the value.
// Otherwise, it stores the value into v.  In that case, v must represent
// a non-nil pointer to data or be an assignable reflect.Value (v.CanSet())

>
http://codereview.appspot.com/4423043/diff/2002/src/pkg/gob/decoder.go#newcod...
> src/pkg/gob/decoder.go:184: panic("gob: DecodeValue of unassignable
> value")
> should this panic? just return an error.

Done.

[r, 2011-04-18 18:27:57]

LGTM
but let's have a separate conversation about New/AllocValue/ValueOf

[rsc, 2011-04-18 18:36:24]

*** Submitted as http://code.google.com/p/go/source/detail?r=c1b858bc04cd ***

changes for more restricted reflect.SetValue

R=golang-dev, r
CC=golang-dev
http://codereview.appspot.com/4423043