code review 4383050: goprotobuf: update to new reflect interface.

goprotobuf: update to new reflect interface.
Also update the compiler golden file.

[r, 2011-04-09 22:52:33]

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

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

[dsymonds, 2011-04-10 00:15:11]

There's lots of cleanup that can be done.
If you want to keep this CL to just the gofixed stuff then feel free to punt on
these.

http://codereview.appspot.com/4383050/diff/1/proto/properties.go
File proto/properties.go (right):

http://codereview.appspot.com/4383050/diff/1/proto/properties.go#newcode233
proto/properties.go:233: switch t1 := typ; t1.Kind() {
This could now just switch on typ.Kind(), and same for the inner switches.

http://codereview.appspot.com/4383050/diff/1/proto/properties.go#newcode249
proto/properties.go:249: case reflect.Int, reflect.Int8, reflect.Int16,
reflect.Int32, reflect.Int64, reflect.Uint, reflect.Uint8, reflect.Uint16,
reflect.Uint32, reflect.Uint64, reflect.Uintptr:
maybe split these out so there's no need for the inner switch? Int32/Uint32 in
one, and Int64/Uint64 in the other.

http://codereview.appspot.com/4383050/diff/1/proto/properties.go#newcode264
proto/properties.go:264: case reflect.Float32, reflect.Float64:
split out?

http://codereview.appspot.com/4383050/diff/1/proto/properties.go#newcode311
proto/properties.go:311: case reflect.Int, reflect.Int8, reflect.Int16,
reflect.Int32, reflect.Int64, reflect.Uint, reflect.Uint8, reflect.Uint16,
reflect.Uint32, reflect.Uint64, reflect.Uintptr:
ditto

http://codereview.appspot.com/4383050/diff/1/proto/properties.go#newcode343
proto/properties.go:343: case reflect.Float32, reflect.Float64:
ditto

http://codereview.appspot.com/4383050/diff/1/proto/text.go
File proto/text.go (right):

http://codereview.appspot.com/4383050/diff/1/proto/text.go#newcode102
proto/text.go:102: if pv := fv; pv.Kind() == reflect.Ptr && pv.IsNil() {
if fv.Kind() == reflect.Ptr && fv.IsNil() {

(and below)

[r, 2011-04-10 01:22:49]

I'm leaning towards doing the minimum to get it through, and clean up
in another pass.  I have a working setup now; without this it won't
even build.

That way we separate automatic fixes from human rewrites for clarity,
and have a testing baseline.

I did need to do a little hand work to get this to go, but only in a
couple of (subtle) places.
-rob

[dsymonds, 2011-04-10 01:26:41]

LGTM

OK.

[rsc, 2011-04-11 19:51:11]

LGTM

can defer any of these to future cleanup

http://codereview.appspot.com/4383050/diff/1/proto/encode.go
File proto/encode.go (right):

http://codereview.appspot.com/4383050/diff/1/proto/encode.go#newcode295
proto/encode.go:295: case reflect.Chan, reflect.Func, reflect.Interface,
reflect.Map, reflect.Ptr, reflect.Slice:
I think it's good to be precise here and test for what you really expect.
So Chan and Func can go and probably Interface too.

http://codereview.appspot.com/4383050/diff/1/proto/properties.go
File proto/properties.go (right):

http://codereview.appspot.com/4383050/diff/1/proto/properties.go#newcode249
proto/properties.go:249: case reflect.Int, reflect.Int8, reflect.Int16,
reflect.Int32, reflect.Int64, reflect.Uint, reflect.Uint8, reflect.Uint16,
reflect.Uint32, reflect.Uint64, reflect.Uintptr:
On 2011/04/10 00:15:11, dsymonds wrote:
> maybe split these out so there's no need for the inner switch? Int32/Uint32 in
> one, and Int64/Uint64 in the other.

Even if not, at least toss the ones that are not expected.

case reflect.Int32, reflect.Int64, reflect.Uint32, reflect.Uint64:

?

http://codereview.appspot.com/4383050/diff/1/proto/properties.go#newcode545
proto/properties.go:545: t = reflect.Typeof(pb)
This looks good to me.
I probably wrote the code it replaces and I can't explain
why it was so complicated.

http://codereview.appspot.com/4383050/diff/1/proto/text_parser.go
File proto/text_parser.go (right):

http://codereview.appspot.com/4383050/diff/1/proto/text_parser.go#newcode389
proto/text_parser.go:389: case reflect.Int, reflect.Int8, reflect.Int16,
reflect.Int32, reflect.Int64:
case reflect.Int32, reflect.Int64:

http://codereview.appspot.com/4383050/diff/1/proto/text_parser.go#newcode436
proto/text_parser.go:436: case reflect.Uint, reflect.Uint8, reflect.Uint16,
reflect.Uint32, reflect.Uint64, reflect.Uintptr:
case reflect.Uint32, reflect.Uint64:

[r, 2011-04-11 19:52:53]

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

goprotobuf: update to new reflect interface.
Also update the compiler golden file.

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