code review 4239052: reflect: add PtrTo, add Value.Addr (old Addr is now Uns...

reflect: add PtrTo, add Value.Addr (old Addr is now UnsafeAddr)

This change makes it possible to take the address of a
struct field or slice element in order to call a method that
requires a pointer receiver.

Existing code that uses the Value.Addr method will have
to change (as gob does in this CL) to call UnsafeAddr instead.

[rsc, 2011-03-03 05:17:33]

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

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

[r, 2011-03-03 05:37:46]

http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/type.go#newco...
src/pkg/reflect/type.go:713: // PtrTo returns the pointer type with element t.
s/element/element type/?
worth an example:
// For instance, if t is the type of Foo, PtrTo returns the type of *Foo.

http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/type.go#newco...
src/pkg/reflect/type.go:758: p.hash = ct.hash*16777619 ^ '*' // one step of
32-bit FNV-1 hash, should be fine
i have no idea this means. none whatsoever.

http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go#newc...
src/pkg/reflect/value.go:71: // Addr returns the address of the value,
represented as a pointer Value.
s/PointerValue/*PtrValue/ ?
but then that makes me ask: why doesn't it return a *PtrValue?
also: who needs this?

http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go#newc...
src/pkg/reflect/value.go:77: // import the "unsafe" package.
fold 76 and 77 together

http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go#newc...
src/pkg/reflect/value.go:118: // - the caller would get the address of a -
move the first - up one line

http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go#newc...
src/pkg/reflect/value.go:817: // If fv represents a method with a bound
receiver,
what is a method with a bound receiver?

http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go#newc...
src/pkg/reflect/value.go:1146: panic("cannot copy pointer obtained via
unexported struct field")
s/via/by/ or /from/  no need for latin

[r, 2011-03-03 05:38:16]

http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/type.go#newco...
src/pkg/reflect/type.go:758: p.hash = ct.hash*16777619 ^ '*' // one step of
32-bit FNV-1 hash, should be fine
On 2011/03/03 05:37:46, r wrote:
> i have no idea this means. none whatsoever.

*what* this means

[rog, 2011-03-03 07:31:19]

LGTM

nice to see - this change means that, for example, the type of RPC methods
could now change from func(*T, *T) os.Error to the more natural
func(T) (T, os.Error)


On 3 March 2011 05:17,  <rsc@golang.org> wrote:
> Reviewers: r,
>
> Message:
> Hello r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> reflect: add PtrTo, add Value.Addr (old Addr is now UnsafeAddr)
>
> This change makes it possible to take the address of a
> struct field or slice element in order to call a method that
> requires a pointer receiver.
>
> Existing code that uses the Value.Addr method will have
> to change (as gob does in this CL) to call UnsafeAddr instead.
>
> Please review this at http://codereview.appspot.com/4239052/
>
> Affected files:
>  M src/pkg/gob/decode.go
>  M src/pkg/gob/encode.go
>  M src/pkg/reflect/all_test.go
>  M src/pkg/reflect/deepequal.go
>  M src/pkg/reflect/type.go
>  M src/pkg/reflect/value.go
>
>
>

[rsc, 2011-03-03 14:22:21]

PTAL

>
http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/type.go#newco...
> src/pkg/reflect/type.go:713: // PtrTo returns the pointer type with
> element t.
> s/element/element type/?
> worth an example:
> // For instance, if t is the type of Foo, PtrTo returns the type of
> *Foo.

Done.
// PtrTo returns the pointer type with element t.
// For example, if t represents type Foo, PtrTo(t) represents *Foo.


>
>
http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/type.go#newco...
> src/pkg/reflect/type.go:758: p.hash = ct.hash*16777619 ^ '*' // one step
> of 32-bit FNV-1 hash, should be fine
> i have no idea this means. none whatsoever.
>
> http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go
> File src/pkg/reflect/value.go (right):
>
>
http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go#newc...
> src/pkg/reflect/value.go:71: // Addr returns the address of the value,
> represented as a pointer Value.
> s/PointerValue/*PtrValue/ ?
> but then that makes me ask: why doesn't it return a *PtrValue?
> also: who needs this?
>
>
http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go#newc...
> src/pkg/reflect/value.go:77: // import the "unsafe" package.
> fold 76 and 77 together
>
>
http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go#newc...
> src/pkg/reflect/value.go:118: // - the caller would get the address of a
> -
> move the first - up one line
>
>
http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go#newc...
> src/pkg/reflect/value.go:817: // If fv represents a method with a bound
> receiver,
> what is a method with a bound receiver?
>
>
http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go#newc...
> src/pkg/reflect/value.go:1146: panic("cannot copy pointer obtained via
> unexported struct field")
> s/via/by/ or /from/  no need for latin
>
> http://codereview.appspot.com/4239052/
>

[rsc, 2011-03-03 14:26:22]

[sorry, gmail imagined that i clicked send a little early on that last one.]

PTAL

omitted items done.

>
http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/type.go#newco...
> src/pkg/reflect/type.go:713: // PtrTo returns the pointer type with
> element t.
> s/element/element type/?
> worth an example:
> // For instance, if t is the type of Foo, PtrTo returns the type of
> *Foo.

// PtrTo returns the pointer type with element t.
// For example, if t represents type Foo, PtrTo(t) represents *Foo.

>
http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/type.go#newco...
> src/pkg/reflect/type.go:758: p.hash = ct.hash*16777619 ^ '*' // one step
> of 32-bit FNV-1 hash, should be fine
> i have no idea this means. none whatsoever.

	s := "*" + *ct.string
	p.string = &s

	// For the type structures linked into the binary, the
	// compiler provides a good hash of the string.
	// Create a good hash for the new string by using
	// the FNV-1 hash's mixing function to combine the
	// old hash and the new "*".
	p.hash = ct.hash*16777619 ^ '*'

> src/pkg/reflect/value.go:71: // Addr returns the address of the value,
> represented as a pointer Value.
> s/PointerValue/*PtrValue/ ?
> but then that makes me ask: why doesn't it return a *PtrValue?
> also: who needs this?

	// Addr returns the address of the value.
	// If the value is not addressable, Addr panics.
	// Addr is typically used to obtain a pointer to a struct field or
slice element
	// in order to call a method that requires a pointer receiver.
	Addr() *PtrValue

>
http://codereview.appspot.com/4239052/diff/4001/src/pkg/reflect/value.go#newc...
> src/pkg/reflect/value.go:817: // If fv represents a method with a bound
> receiver,
> what is a method with a bound receiver?

// Interface returns the fv as an interface value.
// If fv is a method obtained by invoking Value.Method
// (as opposed to Type.Method), Interface cannot return an
// interface value, so it panics.

[r, 2011-03-03 16:48:42]

oh, this means lots of things are possible.

-rob

[r, 2011-03-03 17:36:45]

LGTM

http://codereview.appspot.com/4239052/diff/9/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

http://codereview.appspot.com/4239052/diff/9/src/pkg/reflect/all_test.go#newc...
src/pkg/reflect/all_test.go:1414: typ = typ.(*PtrType).Elem()
it's only a test and it's never going to fail, but this should probably be a
comma ok
but a crash will put the error here too, so it's up to you

[rog, 2011-03-03 18:02:20]

On 3 March 2011 16:48, Rob 'Commander' Pike <r@golang.org> wrote:
> oh, this means lots of things are possible.

for instance... does gob need to be unsafe any more?

[rsc, 2011-03-03 18:17:39]

> for instance... does gob need to be unsafe any more?

yes.

gob is unsafe so that it can precompute some of the work
and avoid many allocations that reflect performs.
the change in this CL doesn't address any of the
reasons that gob uses unsafe.  (what this CL enables,
gob has never done.)

in a week or so, i am going to send out a proposal for a new reflect API
that would eliminate almost all the allocations and probably
be nicer to use.  at that point it might be worth looking
at whether gob could be as fast using reflect.

russ

[rsc, 2011-03-03 18:20:18]

*** Submitted as 5e9b7195ed0d ***

reflect: add PtrTo, add Value.Addr (old Addr is now UnsafeAddr)

This change makes it possible to take the address of a
struct field or slice element in order to call a method that
requires a pointer receiver.

Existing code that uses the Value.Addr method will have
to change (as gob does in this CL) to call UnsafeAddr instead.

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

[rog, 2011-03-03 18:27:57]

On 3 March 2011 18:17, Russ Cox <rsc@golang.org> wrote:
> in a week or so, i am going to send out a proposal for a new reflect API
> that would eliminate almost all the allocations and probably
> be nicer to use.

that's quite exciting. things will speed up.