code review 7085051: encoding/gob: handle encoding of different indirects of...

encoding/gob: handle encoding of different indirects of GobEncoder

Fixes issue 4647.

[kevlar, 2013-01-12 20:51:11]

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

I'd like you to review this change to
http://code.google.com/p/go

[r, 2013-01-13 17:04:10]

feel free to leave this for me. i'll get to it, but not right away.

https://codereview.appspot.com/7085051/diff/4001/src/pkg/encoding/gob/encoder.go
File src/pkg/encoding/gob/encoder.go (right):

https://codereview.appspot.com/7085051/diff/4001/src/pkg/encoding/gob/encoder...
src/pkg/encoding/gob/encoder.go:94: t = t.Elem()
this is a substantial change to a fundamental routine to fix a specific problem.
i don't believe it's the best fix.

instead, i'd make the relevant change to the gobEncoder code itself. in fact, it
might just be that you want to s/user/base/ in the call there, since that's
close to what you're doing here.

[kevlar, 2013-01-13 17:40:50]

PTAL

On 2013/01/13 17:04:10, r wrote:
> feel free to leave this for me. i'll get to it, but not right away.
I can leave it for you if you think the change requires more than this.

>
https://codereview.appspot.com/7085051/diff/4001/src/pkg/encoding/gob/encoder.go
> File src/pkg/encoding/gob/encoder.go (right):
> 
>
https://codereview.appspot.com/7085051/diff/4001/src/pkg/encoding/gob/encoder...
> src/pkg/encoding/gob/encoder.go:94: t = t.Elem()
> this is a substantial change to a fundamental routine to fix a specific
problem.
> i don't believe it's the best fix.
I was rather hoping that was the case; it didn't feel very clean.

> instead, i'd make the relevant change to the gobEncoder code itself. in fact,
it
> might just be that you want to s/user/base/ in the call there, since that's
> close to what you're doing here.
Aha!  Your instinct is correct.  I had seen that in the other call to
sendActualType but it didn't click.  All tests (including the new one) pass with
this change.

[r, 2013-01-13 18:13:14]

https://codereview.appspot.com/7085051/diff/8001/src/pkg/encoding/gob/encoder.go
File src/pkg/encoding/gob/encoder.go (right):

https://codereview.appspot.com/7085051/diff/8001/src/pkg/encoding/gob/encoder...
src/pkg/encoding/gob/encoder.go:140: // we need to tell the other side that this
exact type is a GobEncoder.
s/this exact/the base/

https://codereview.appspot.com/7085051/diff/8001/src/pkg/encoding/gob/gobencd...
File src/pkg/encoding/gob/gobencdec_test.go (right):

https://codereview.appspot.com/7085051/diff/8001/src/pkg/encoding/gob/gobencd...
src/pkg/encoding/gob/gobencdec_test.go:380: tests := []struct {
this is cute but none of the other tests here look like this. you're using
interfaces and such anyway so it's not quite working for you; just write the
four cases out.

[kevlar, 2013-01-13 18:50:20]

PTAL

https://codereview.appspot.com/7085051/diff/8001/src/pkg/encoding/gob/encoder.go
File src/pkg/encoding/gob/encoder.go (right):

https://codereview.appspot.com/7085051/diff/8001/src/pkg/encoding/gob/encoder...
src/pkg/encoding/gob/encoder.go:140: // we need to tell the other side that this
exact type is a GobEncoder.
On 2013/01/13 18:13:14, r wrote:
> s/this exact/the base/

Done.

https://codereview.appspot.com/7085051/diff/8001/src/pkg/encoding/gob/gobencd...
File src/pkg/encoding/gob/gobencdec_test.go (right):

https://codereview.appspot.com/7085051/diff/8001/src/pkg/encoding/gob/gobencd...
src/pkg/encoding/gob/gobencdec_test.go:380: tests := []struct {
On 2013/01/13 18:13:14, r wrote:
> this is cute but none of the other tests here look like this. you're using
> interfaces and such anyway so it's not quite working for you; just write the
> four cases out.

Done.

I also wasn't sure whether to put this at the bottom with the other bugs or up
here with the other GobEncoder tests.

[r, 2013-01-14 05:03:56]

LGTM

can someone check this in please? i don't have a current client.

[adg, 2013-01-14 05:07:21]

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

encoding/gob: handle encoding of different indirects of GobEncoder

Fixes issue 4647.

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

Committer: Andrew Gerrand <adg@golang.org>

[adg, 2013-01-14 05:07:21]

sure.

[remyoudompheng, 2013-07-20 20:08:55]

R=close