code review 7393045: cmd/gc, reflect, runtime: switch to indirect func value...

cmd/gc, reflect, runtime: switch to indirect func value representation

Step 1 of http://golang.org/s/go11func.

[rsc, 2013-02-21 19:22:58]

Hello golang-dev@googlegroups.com,

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

[rsc, 2013-02-21 19:28:15]

Looks like a lot, but it's only +316, -89.

Russ

[r, 2013-02-21 20:04:15]

LGTM

reflect code looks good except for one unclear comment.
i will leave the compiler and runtime changes for others.

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

https://codereview.appspot.com/7393045/diff/18001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:1220: // result is zero if and only if v's value is a
nil func.
define 'nil func' better. it's an implementation detail, not how the user sees
the value.

[rsc, 2013-02-21 20:14:11]

On Thu, Feb 21, 2013 at 3:04 PM, <r@golang.org> wrote:

> https://codereview.appspot.**com/7393045/diff/18001/src/**
>
pkg/reflect/value.go#**newcode1220<https://codereview.appspot.com/7393045/diff/18001/src/pkg/reflect/value.go#newcode1220>
> src/pkg/reflect/value.go:1220: // result is zero if and only if v's
> value is a nil func.
> define 'nil func' better. it's an implementation detail, not how the
> user sees the value.
>

I meant what the user sees.

var f func()
v := reflect.ValueOf(f)
v.Pointer() == 0.

Russ

[r, 2013-02-21 20:33:40]

if you just said func Value rather than func, it would help

[DMorsing, 2013-02-21 20:57:17]

Did a quick glance of gc/6g/runtime code. Like you said, it's mostly small stuff
like changing the indirection on a function pointer. Don't feel qualified enough
to give a LGTM though.

[rsc, 2013-02-21 20:59:53]

On Thu, Feb 21, 2013 at 3:33 PM, Rob Pike <r@golang.org> wrote:

> if you just said func Value rather than func, it would help
>

Done.

// If v's Kind is Func, the returned pointer is an underlying
// code pointer, but not necessarily enough to identify a
// single function uniquely. The only guarantee is that the
// result is zero if and only if v is a nil func Value.

[remyoudompheng, 2013-02-21 21:32:44]

couldn't find any other remark

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

https://codereview.appspot.com/7393045/diff/25001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:400: fn = *(*unsafe.Pointer)(v.val)
is this ever possible?

https://codereview.appspot.com/7393045/diff/25001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:1236: p = *(*unsafe.Pointer)(p)
since func values are pointer-sized, I suppose this test can be removed?

[DMorsing, 2013-02-21 21:39:03]

Scanning over this again, i'm going to say LGTM for 6g/gc/runtime

[rsc, 2013-02-21 21:56:16]

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

https://codereview.appspot.com/7393045/diff/25001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:400: fn = *(*unsafe.Pointer)(v.val)
On 2013/02/21 21:32:44, remyoudompheng wrote:
> is this ever possible?

Yes. Any addressable value records the pointer to the actual data, so that it
can be Set. That's this case.

https://codereview.appspot.com/7393045/diff/25001/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:1236: p = *(*unsafe.Pointer)(p)
On 2013/02/21 21:32:44, remyoudompheng wrote:
> since func values are pointer-sized, I suppose this test can be removed?

Same.

[rsc, 2013-02-21 22:01:17]

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

cmd/gc, reflect, runtime: switch to indirect func value representation

Step 1 of http://golang.org/s/go11func.

R=golang-dev, r, daniel.morsing, remyoudompheng
CC=golang-dev
https://codereview.appspot.com/7393045

[remyoudompheng, 2013-02-21 23:15:06]

Windows builders didn't like it.

Rémy.

[brainman, 2013-02-22 00:54:10]

On Friday, 22 February 2013 10:15:05 UTC+11, Rémy wrote:
>
> Windows builders didn't like it. 
>
>
I think I know why. Windows callback runtime code uses refs to go 
functions. These are changed. We need to change windows callback 
accordingly. I will send a fix.

Alex 

[brainman, 2013-02-22 01:08:10]

https://codereview.appspot.com/7393048/

[rsc, 2013-02-22 15:20:54]

Thank you for fixing this, and apologies for not catching it. I was sure I
hadn't done anything OS-specific. As penance, I set up a Windows Server
2012 machine on Amazon AWS that I can turn on for an hour or two at a time
to fix things like this (at least for windows/amd64) in the future.

Russ