code review 837042: Add support for XML marshalling embedded structs.

Add support for XML marshalling embedded structs.

The newly added methods to the reflection package could be placed in the xml package but that would require duplicating the bulk of the logic which is in the private unmarshal method.

The new _test file also contains the case rsc raised on the mailing list re. filed name conflict.

[rsn, 2010-03-30 11:59:33]

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

I'd like you to review this change.

[rsc1, 2010-04-11 20:26:02]

Looks pretty good.
Sorry for the delay.

http://codereview.appspot.com/837042/diff/6/2001
File src/pkg/reflect/type.go (right):

http://codereview.appspot.com/837042/diff/6/2001#newcode1
src/pkg/reflect/type.go:1: // Copyright 2009-2010 The Go Authors. All rights
reserved.
please don't edit the copyright years.
the original year is fine.

http://codereview.appspot.com/837042/diff/6/2001#newcode511
src/pkg/reflect/type.go:511: func (t *StructType) fieldByName(name string,
ignoreCase bool, mark map[*StructType]bool, depth int) (ff StructField, fd int)
{
This doesn't feel like the right solution.
Maybe instead the two params name string, ignoreCase bool
should be a single match func(string) bool
and then this can be called fieldByNameFunc.

http://codereview.appspot.com/837042/diff/6/2001#newcode580
src/pkg/reflect/type.go:580: if ff, fd := t.fieldByName(name, false,
make(map[*StructType]bool), 0); fd < inf {
return t.FieldByNameFunc(func(s string) bool { return s == name })

http://codereview.appspot.com/837042/diff/6/2001#newcode589
src/pkg/reflect/type.go:589: func (t *StructType) FieldByNameIgnoreCase(name
string) (f StructField, present bool) {
func (t *StructType) FieldByNameFunc(match func(string) bool)...

http://codereview.appspot.com/837042/diff/6/2002
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/837042/diff/6/2002#newcode1
src/pkg/reflect/value.go:1: // Copyright 2009-2010 The Go Authors. All rights
reserved.
undo

http://codereview.appspot.com/837042/diff/6/2002#newcode1254
src/pkg/reflect/value.go:1254: // FieldByNameIgnoreCase returns the struct field
with a name matching
Same change.

http://codereview.appspot.com/837042/diff/6/2003
File src/pkg/xml/embed_test.go (right):

http://codereview.appspot.com/837042/diff/6/2003#newcode7
src/pkg/xml/embed_test.go:7: import (
If there's just one import, can use a single line instead of ( ) form.

http://codereview.appspot.com/837042/diff/6/2004
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/837042/diff/6/2004#newcode1
src/pkg/xml/read.go:1: // Copyright 2009-2010 The Go Authors.  All rights
reserved.
undo

http://codereview.appspot.com/837042/diff/6/2004#newcode338
src/pkg/xml/read.go:338: // NOTE (rsn) - should also handle the case of Any
There is existing code that uses "Any".
Please put that support back.
It should be as easy as looking up the name "Any".

http://codereview.appspot.com/837042/diff/6/2004#newcode341
src/pkg/xml/read.go:341: // skip if name is not public
Can put this in the finding function now.

[rsn, 2010-04-12 17:34:54]

hello Russ,

On 2010/04/11 20:26:02, rsc1 wrote:
> Looks pretty good.
> Sorry for the delay...

no worries.  thanks for taking the time to review this CL.

the next patch soon to be mailed contains the updated code and an added
test-case for testing Any assignments.


cheers;
rsn

[rsn, 2010-04-12 17:35:10]

http://codereview.appspot.com/837042/diff/6/2001
File src/pkg/reflect/type.go (right):

http://codereview.appspot.com/837042/diff/6/2001#newcode1
src/pkg/reflect/type.go:1: // Copyright 2009-2010 The Go Authors. All rights
reserved.
On 2010/04/11 20:26:03, rsc1 wrote:
> please don't edit the copyright years.
> the original year is fine.
> 

Done.

http://codereview.appspot.com/837042/diff/6/2001#newcode511
src/pkg/reflect/type.go:511: func (t *StructType) fieldByName(name string,
ignoreCase bool, mark map[*StructType]bool, depth int) (ff StructField, fd int)
{
On 2010/04/11 20:26:03, rsc1 wrote:
> This doesn't feel like the right solution.
> Maybe instead the two params name string, ignoreCase bool
> should be a single match func(string) bool
> and then this can be called fieldByNameFunc.
> 

Done.

http://codereview.appspot.com/837042/diff/6/2001#newcode580
src/pkg/reflect/type.go:580: if ff, fd := t.fieldByName(name, false,
make(map[*StructType]bool), 0); fd < inf {
On 2010/04/11 20:26:03, rsc1 wrote:
> return t.FieldByNameFunc(func(s string) bool { return s == name })
> 

Done.

http://codereview.appspot.com/837042/diff/6/2001#newcode589
src/pkg/reflect/type.go:589: func (t *StructType) FieldByNameIgnoreCase(name
string) (f StructField, present bool) {
On 2010/04/11 20:26:03, rsc1 wrote:
> func (t *StructType) FieldByNameFunc(match func(string) bool)...
> 

Done.

http://codereview.appspot.com/837042/diff/6/2002
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/837042/diff/6/2002#newcode1
src/pkg/reflect/value.go:1: // Copyright 2009-2010 The Go Authors. All rights
reserved.
On 2010/04/11 20:26:03, rsc1 wrote:
> undo
> 

Done.

http://codereview.appspot.com/837042/diff/6/2002#newcode1254
src/pkg/reflect/value.go:1254: // FieldByNameIgnoreCase returns the struct field
with a name matching
On 2010/04/11 20:26:03, rsc1 wrote:
> Same change.
> 

Done.

http://codereview.appspot.com/837042/diff/6/2003
File src/pkg/xml/embed_test.go (right):

http://codereview.appspot.com/837042/diff/6/2003#newcode7
src/pkg/xml/embed_test.go:7: import (
On 2010/04/11 20:26:03, rsc1 wrote:
> If there's just one import, can use a single line instead of ( ) form.
> 

Done.

also added a new test case for Any field.

http://codereview.appspot.com/837042/diff/6/2004
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/837042/diff/6/2004#newcode1
src/pkg/xml/read.go:1: // Copyright 2009-2010 The Go Authors.  All rights
reserved.
On 2010/04/11 20:26:03, rsc1 wrote:
> undo
> 

Done.

http://codereview.appspot.com/837042/diff/6/2004#newcode338
src/pkg/xml/read.go:338: // NOTE (rsn) - should also handle the case of Any
On 2010/04/11 20:26:03, rsc1 wrote:
> There is existing code that uses "Any".
> Please put that support back.
> It should be as easy as looking up the name "Any".
> 
> 

Done.

http://codereview.appspot.com/837042/diff/6/2004#newcode341
src/pkg/xml/read.go:341: // skip if name is not public
On 2010/04/11 20:26:03, rsc1 wrote:
> Can put this in the finding function now.
> 

Done.

[rsn, 2010-04-12 17:35:54]

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

Please take another look.

[rsn, 2010-04-12 17:44:26]

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

Please take another look.

[rsc1, 2010-04-13 20:59:59]

Looks good.  Small suggestion in xml code.

http://codereview.appspot.com/837042/diff/15001/16004
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/837042/diff/15001/16004#newcode348
src/pkg/xml/read.go:348: f, found := styp.FieldByNameFunc(match)
Can avoid the duplicated body here.

f, found := styp.FieldByNameFunc(match)
if !found {
    f, found = styp.FieldByName("Any")
}
if found {
    ...
    continue Loop
}

[rsn, 2010-04-14 09:25:45]

hello Russ,

i'll mail the amended version soon.

thanks again for reviewing this CL!


cheers;
rsn

http://codereview.appspot.com/837042/diff/15001/16004
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/837042/diff/15001/16004#newcode348
src/pkg/xml/read.go:348: f, found := styp.FieldByNameFunc(match)
On 2010/04/13 20:59:59, rsc1 wrote:
> Can avoid the duplicated body here.
> 
> f, found := styp.FieldByNameFunc(match)
> if !found {
>     f, found = styp.FieldByName("Any")
> }
> if found {
>     ...
>     continue Loop
> }
> 

nice!  Done.

[rsn, 2010-04-14 09:26:55]

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

Please take another look.

[rsc, 2010-04-18 21:48:16]

LGTM

[rsc, 2010-04-18 22:22:39]

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

reflect: add FieldByNameFunc
xml: add support for XML marshalling embedded structs.

R=rsc
CC=golang-dev
http://codereview.appspot.com/837042

Committer: Russ Cox <rsc@golang.org>