code review 5694044: encoding/xml: handle anonymous pointer fields

encoding/xml: handle anonymous pointer fields

This CL makes

    type T struct { *U }

behave in a similar way to:

    type T struct { U }

Fixes issue 3108.

[niemeyer, 2012-02-23 03:19:04]

Hello golang-dev@googlegroups.com,

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

[rsc, 2012-02-23 03:25:28]

I'm sorry, but I think it's too late.  Let's let xml sit and get
pounded on.  We can fix this after Go 1.  It is a rare and relatively
unimportant case, and the fact that the fields are ignored means that
we can fix it without changing behavior too much.

[niemeyer, 2012-04-17 01:29:48]

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

Please take another look.

[rsc, 2012-04-25 02:52:29]

Can you please describe what this CL does before I read the code?
I was kind of hoping for a discussion of what "handle" might mean.
It's not entirely obvious to me.

The CL description should also give more detail, and presumably
the package document comment needs editing.  This is a significant
API change but from a quick glance you wouldn't realize that.

Thanks.
Russ

[gustavo_niemeyer.net, 2012-04-25 03:01:35]

On Tue, Apr 24, 2012 at 23:52, Russ Cox <rsc@golang.org> wrote:
> Can you please describe what this CL does before I read the code?
> I was kind of hoping for a discussion of what "handle" might mean.
> It's not entirely obvious to me.
>
> The CL description should also give more detail, and presumably
> the package document comment needs editing.  This is a significant
> API change but from a quick glance you wouldn't realize that.

Maybe that's because I don't really see that as a significant API change.

This CL makes

    type Foo struct {
        *Bar
    }

behave as

    type Foo struct {
        Bar
    }

does today.

[rsc, 2012-04-25 03:26:20]

I see.  I thought xml was ignoring all anonymous fields.
At the very least this comment needs updating, no?

//   * A non-pointer anonymous struct field is handled as if the
//      fields of its value were part of the outer struct.

Russ

[gustavo_niemeyer.net, 2012-04-25 03:47:12]

On Wed, Apr 25, 2012 at 00:25, Russ Cox <rsc@golang.org> wrote:
> I see.  I thought xml was ignoring all anonymous fields.
> At the very least this comment needs updating, no?
>
> //   * A non-pointer anonymous struct field is handled as if the
> //      fields of its value were part of the outer struct.

Good catch.

[niemeyer, 2012-04-25 05:05:01]

Hello golang-dev@googlegroups.com, rsc@golang.org, gustavo@niemeyer.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[niemeyer, 2012-04-30 22:54:00]

ping

[rsc, 2012-05-01 22:49:46]

LGTM

http://codereview.appspot.com/5694044/diff/15001/src/pkg/encoding/xml/typeinf...
File src/pkg/encoding/xml/typeinfo.go (right):

http://codereview.appspot.com/5694044/diff/15001/src/pkg/encoding/xml/typeinf...
src/pkg/encoding/xml/typeinfo.go:70: if t.Kind() == reflect.Ptr &&
t.Elem().Kind() == reflect.Struct {
You can make the cases less special by doing

if t.Kind() == reflect.Ptr {
    t = t.Elem()
}
if t.Kind() != reflect.Struct {
    continue
}

http://codereview.appspot.com/5694044/diff/15001/src/pkg/encoding/xml/typeinf...
src/pkg/encoding/xml/typeinfo.go:334: // Value returns v's field value
corresponding to finfo.
Can you please call this 'value', so that it's obvious at call sites that it is
not an method usable by clients of this package?

[niemeyer, 2012-05-17 02:21:44]

*** Submitted as http://code.google.com/p/go/source/detail?r=7339b16a1f58 ***

encoding/xml: handle anonymous pointer fields

This CL makes

    type T struct { *U }

behave in a similar way to:

    type T struct { U }

Fixes issue 3108.

R=golang-dev, rsc, gustavo
CC=golang-dev
http://codereview.appspot.com/5694044

http://codereview.appspot.com/5694044/diff/15001/src/pkg/encoding/xml/typeinf...
File src/pkg/encoding/xml/typeinfo.go (right):

http://codereview.appspot.com/5694044/diff/15001/src/pkg/encoding/xml/typeinf...
src/pkg/encoding/xml/typeinfo.go:70: if t.Kind() == reflect.Ptr &&
t.Elem().Kind() == reflect.Struct {
On 2012/05/01 22:49:46, rsc wrote:
> You can make the cases less special by doing
> 
> if t.Kind() == reflect.Ptr {
>     t = t.Elem()
> }
> if t.Kind() != reflect.Struct {
>     continue
> }

Done.

http://codereview.appspot.com/5694044/diff/15001/src/pkg/encoding/xml/typeinf...
src/pkg/encoding/xml/typeinfo.go:334: // Value returns v's field value
corresponding to finfo.
On 2012/05/01 22:49:46, rsc wrote:
> Can you please call this 'value', so that it's obvious at call sites that it
is
> not an method usable by clients of this package?
> 

Done.