code review 4432051: goprotobuf: update for new Set semantics for reflect.Value.

goprotobuf: update for new Set semantics for reflect.Value.

[r, 2011-04-18 19:46:11]

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

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

[rsc, 2011-04-18 19:57:30]

http://codereview.appspot.com/4432051/diff/1/proto/extensions.go
File proto/extensions.go (right):

http://codereview.appspot.com/4432051/diff/1/proto/extensions.go#newcode179
proto/extensions.go:179: // SetExtensions assumes it can call UnsafeAddr on any
Value
s/Extensions/Extension/

http://codereview.appspot.com/4432051/diff/1/proto/extensions.go#newcode185
proto/extensions.go:185: // TODO: this is a copy of a function from gob. Move it
to reflect.
I don't believe this function belongs in reflect.

It's a clumsy hack to make up for proto insisting on
being able to take the address of values that need not
be addressable.

That said, I am pretty confused about why SetExtension
is using v.UnsafeAddr() / unsafeAddr(v) instead of v.Pointer(),
unless value holds a structure rather than a pointer to a
structure (seems unlikely).

This code might be completely unnecessary, and perhaps
SetExtension should be using v.Pointer(), but if so it was
buggy before.  The new code is as correct (or not) as the old
code, so LGTM.

I don't see any calls to SetExtension in the tests.

Leave for dsymonds.

[rsc, 2011-04-18 19:57:36]

LGTM

[r, 2011-04-18 20:13:07]

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

goprotobuf: update for new Set semantics for reflect.Value.

R=rsc
CC=dsymonds, golang-dev
http://codereview.appspot.com/4432051

[dsymonds, 2011-04-18 23:39:33]

FYI

On Tue, Apr 19, 2011 at 5:57 AM,  <rsc@golang.org> wrote:

> I don't see any calls to SetExtension in the tests.

It's in compiler/testdata/extension_test.go because of a dependence on
the compiler.


Dave.