code review 7906043: reflect: implement method values

reflect: implement method values

Fixes issue 1517.

[rsc, 2013-03-20 21:38:31]

Hello golang-dev@googlegroups.com,

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

[r, 2013-03-20 21:59:17]

https://codereview.appspot.com/7906043/diff/5001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/7906043/diff/5001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:570: panic("reflect: broken Value")
could this be more informative? all it says, really, is "panic: broken".
maybe panic("reflect: method index out of range doing " + op)?
suggestions welcome

https://codereview.appspot.com/7906043/diff/5001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:600: // for a function of the given type. The rcvr bool
specifes whether
specifIes

https://codereview.appspot.com/7906043/diff/5001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:601: // a one-word receiver should be included in the
overall total.
s/overall//

[rsc, 2013-03-21 15:32:39]

PTAL

https://codereview.appspot.com/7906043/diff/5001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/7906043/diff/5001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:570: panic("reflect: broken Value")
On 2013/03/20 21:59:17, r wrote:
> could this be more informative? all it says, really, is "panic: broken".
> maybe panic("reflect: method index out of range doing " + op)?
> suggestions welcome

This code was moved from elsewhere in the file. It's not supposed to be able to
happen. I'll change it to
 reflect: internal error: invalid method index.

https://codereview.appspot.com/7906043/diff/5001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:600: // for a function of the given type. The rcvr bool
specifes whether
On 2013/03/20 21:59:17, r wrote:
> specifIes

Done.

https://codereview.appspot.com/7906043/diff/5001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:601: // a one-word receiver should be included in the
overall total.
On 2013/03/20 21:59:17, r wrote:
> s/overall//

Done.

[r, 2013-03-21 16:24:03]

LGTM

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/all_test.go...
src/pkg/reflect/all_test.go:1573: t.Errorf("Value Method returned %d; want 250",
i)
for another CL maybe: all these things callling the same method and returning
the same value weakens the test. the same issue occurs elsewhere in this file

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/all_test.go...
src/pkg/reflect/all_test.go:1610: }{p}
var x interface{Dist(int) int} = p
pv = reflect.ValueOf(&x).Elem()

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/makefunc.go
File src/pkg/reflect/makefunc.go (right):

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/makefunc.go...
src/pkg/reflect/makefunc.go:96: // actual code address. (A Go func is a pointer
s/func/func value/

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:574: panic(op + " of unexported method")
put "reflect: " on the beginning of the op panics

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:617: total = (total + ptrSize - 1) &^ (ptrSize - 1)
this tricky burst appears 4 times and probably more elsewhere. would be nice as
a separate function, one that would probably inline.

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:636: // round to pointer total
but only this one deserves a comment?

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:637: total = (total + ptrSize - 1) &^ (ptrSize - 1)
this could be in an else.

[rsc, 2013-03-21 20:58:17]

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/all_test.go...
src/pkg/reflect/all_test.go:1573: t.Errorf("Value Method returned %d; want 250",
i)
On 2013/03/21 16:24:03, r wrote:
> for another CL maybe: all these things callling the same method and returning
> the same value weakens the test. the same issue occurs elsewhere in this file

Done. Found a bug in the test.

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/all_test.go...
src/pkg/reflect/all_test.go:1610: }{p}
On 2013/03/21 16:24:03, r wrote:
> var x interface{Dist(int) int} = p
> pv = reflect.ValueOf(&x).Elem()

Done.

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/makefunc.go
File src/pkg/reflect/makefunc.go (right):

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/makefunc.go...
src/pkg/reflect/makefunc.go:96: // actual code address. (A Go func is a pointer
On 2013/03/21 16:24:03, r wrote:
> s/func/func value/

Done.

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:574: panic(op + " of unexported method")
On 2013/03/21 16:24:03, r wrote:
> put "reflect: " on the beginning of the op panics

Done.

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:617: total = (total + ptrSize - 1) &^ (ptrSize - 1)
On 2013/03/21 16:24:03, r wrote:
> this tricky burst appears 4 times and probably more elsewhere. would be nice
as
> a separate function, one that would probably inline. 

Done.

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:636: // round to pointer total
On 2013/03/21 16:24:03, r wrote:
> but only this one deserves a comment?

Done.

https://codereview.appspot.com/7906043/diff/14001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:637: total = (total + ptrSize - 1) &^ (ptrSize - 1)
On 2013/03/21 16:24:03, r wrote:
> this could be in an else.

I'd rather not tie the two.

[rsc, 2013-03-21 20:59:19]

*** Submitted as https://code.google.com/p/go/source/detail?r=150c2fdf0c5e ***

reflect: implement method values

Fixes issue 1517.

R=golang-dev, r
CC=golang-dev
https://codereview.appspot.com/7906043