code review 5503078: xml: major Go 1 fixup

xml: major Go 1 fixup

This CL improves the xml package in the following ways:

- makes its interface match established conventions
- brings Marshal and Unmarshal closer together
- fixes a large number of bugs and adds tests
- improves speed significantly
- organizes and simplifies the code

Fixes issue 2426.
Fixes issue 2406.
Fixes issue 1989.

What follows is a detailed list of those changes.

- All matching is case sensitive without special processing
  to the field name or xml tag in an attempt to match them.
  Customize the field tag as desired to match the correct XML
  elements.

- Flags are ",flag" rather than "flag". The names "attr",
  "chardata", etc, may be used to name actual XML elements.

- Overriding of attribute names is possible with "name,attr".

- Attribute fields are marshalled properly if they have
  non-string types. Previously they were unmarshalled, but were
  ignored at marshalling time.

- Comment fields tagged with ",comment" are marshalled properly,
  rather than being marshalled as normal fields.

- The handling of the Any field has been replaced by the ",any"
  flag to avoid unexpected results when using the field name for
  other purposes, and has also been fixed to interact properly
  with name paths. Previously the feature would not function
  if any field in the type had a name path in its tag.

- Embedded struct support fixed and cleaned so it works when
  marshalling and also when using field paths deeper than one level.

- Conflict reporting on field names have been expanded to cover
  all fields. Previously it'd catch only conflicts of paths
  deeper than one level. Also interacts correctly with embedded
  structs now.

- A trailing '>' is disallowed in xml tags. It used to be
  supported for removing the ambiguity between "attr" and "attr>",
  but the marshalling support for that was broken, and it's now
  unnecessary. Use "name" instead of "name>".

- Fixed docs to point out that a XMLName doesn't have to be
  an xml.Name (e.g. a struct{} is a good fit too). The code was
  already working like that.

- Fixed asymmetry in the precedence of XML element names between
  marshalling and unmarshalling. Marshal would consider the XMLName
  of the field type before the field tag, while unmarshalling would
  do the opposite. Now both respect the tag of the XMLName field
  first, and a nice error message is provided in case an attempt
  is made to name a field with its tag in a way that would
  conflict with the underlying type's XMLName field.

- Do not marshal broken "<???>" tags when in doubt. Use the type
  name, and error out if that's not possible.

- Do not break down unmarshalling if there's an interface{} field
  in a struct.

- Significant speed boost due to caching of type metadata and
  overall allocation clean ups. The following timings reflect
  processing of the the atom test data:

  Old:

  BenchmarkMarshal           50000             48798 ns/op
  BenchmarkUnmarshal          5000            357174 ns/op

  New:

  BenchmarkMarshal          100000             19799 ns/op
  BenchmarkUnmarshal         10000            128525 ns/op

[niemeyer, 2011-12-29 05:49:00]

Hello golang-dev@googlegroups.com,

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

[cw, 2011-12-29 06:10:30]

Wow, this is heroic and really great to see.  A quick glance over this right now
and it looks great.

I'll patch it locally and test it out on a few things here.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/atom_tes...
File src/pkg/encoding/xml/atom_test.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/atom_tes...
src/pkg/encoding/xml/atom_test.go:28: `<title>Example Feed</title>` +
why the case change here?

i agree the change makes it seem cleaner and more natural but it isn't necessary
and maybe could be a separate future CL?

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:147: // 3. type name
what about

XMLname ...
field tag
field name
type name
?

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:249: if bytes.Index(comment, []byte("--")) >= 0
{
how strictly does this have to be enforced?

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read_tes...
File src/pkg/encoding/xml/read_test.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read_tes...
src/pkg/encoding/xml/read_test.go:82: Title   string  `xml:"title"`
if we don't tweak the case of the test data some of this won't be necessary

[niemeyer, 2011-12-29 06:19:47]

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/atom_tes...
File src/pkg/encoding/xml/atom_test.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/atom_tes...
src/pkg/encoding/xml/atom_test.go:28: `<title>Example Feed</title>` +
> why the case change here?

Because the handling of casing is changing in this CL. Tests don't pass if we
don't fix them.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:147: // 3. type name
On 2011/12/29 06:10:30, cw wrote:
> what about
> 
> XMLname ...
> field tag
> field name
> type name
> ?

That's exactly how it is.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:249: if bytes.Index(comment, []byte("--")) >= 0
{
> how strictly does this have to be enforced?

It's a MUST in the specification.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read_tes...
File src/pkg/encoding/xml/read_test.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read_tes...
src/pkg/encoding/xml/read_test.go:82: Title   string  `xml:"title"`
> if we don't tweak the case of the test data some of this won't be necessary

Again, the handling of casing is changing in this CL. The tests were fixed
rather than arbitrarily changed.

[cw, 2011-12-29 23:04:28]

LGMT

After fixing some local bugs am now using this without any noticeable problems.

[gustavo_niemeyer.net, 2011-12-29 23:26:31]

> LGMT
>
> After fixing some local bugs am now using this without any noticeable
> problems.

Sweet. Thanks a lot for putting it to use.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[kevlar, 2011-12-29 23:35:57]

Ah, you are my hero.

Though I'd like this even better if it also fixed 2626 ;-).

Just a few nits about element name handling and I want omitempty!

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:50: //     - the name of the marshalled type
Technically, I think your code uses the name of the first concrete type it finds
within the marshaled value; the distinction is subtle but there.  Thus, if you
have:

type Lecturer interface {
  Lecture()
}

type Instructor struct{Name string}
func (Instructor) Lecture() {}

type Professor struct{Name string}
func (Professor) Lecture() {}
func (Professor) Research() {}

var Lecturer x
x = Instructor{"Spam"}
Marshall(x) -> "<Instructor><Name>Spam</Name></Instructor>"
x = Professor{"Eggs"}
Marshall(x) -> "<Professor><Name>Eggs</Name></Professor>"

I think the least surprising behavior would be to have Lecturer in both cases,
though it's far from cut-and-dried.

This may be why rsc wanted me to use ??? in the first place.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:145: // 1. XMLName field in underlying struct;
This seems wrong... The documentation above applies to the top-level value's
element name selection.  This applies to child elements.  If you have two fields
of type X and X has an XMLName, I think they will both me marshaled with the
same name instead of the field names.

I think it should be
Top-level:
- user-provided
- XMLName tag
- XMLName value
- type name

Inner value:
- field name/tag in the struct field
- XMLName tag
- XMLName value

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:252: p.WriteString("<!--")
is <!--- --> valid?  <!---->?  I can never remember the specifics.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go
File src/pkg/encoding/xml/read.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:235: // For now, simply ignore the field.
I would skip the field if it has no value; if it does have a value, continue to
traverse into it.  This would be analogous to JSON, where you can populate
interface values and be able to unmarshal into it properly.  Of course, JSON
creates a map[string]interface{} or []interface{} for you if there is no value,
but that's JSON.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo.go
File src/pkg/encoding/xml/typeinfo.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:39: //fOmitEmpty
aw, I really like omitempty...  it's not that hard, either, and now that we have
struct equality it's well-defined.

[niemeyer, 2011-12-30 00:48:00]

> Though I'd like this even better if it also fixed 2626 ;-).

I've looked at it, but it sounds like an independent change that is worth its
own debate and CL.

Thanks for the review Kyle. Some feedback follows.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:50: //     - the name of the marshalled type
> var Lecturer x
> x = Instructor{"Spam"}
> Marshall(x) -> "<Instructor><Name>Spam</Name></Instructor>"
(...)
> I think the least surprising behavior would be to have Lecturer in both cases,

There's no possible way to have Lecturer in the case above, since what's being
provided to the function is the real value with its real type under an
interface{}. The suggested documentation is correct in this case: it's the name
of the _marshalled type_. Lecturer is not being marshalled above. Instructor is.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:145: // 1. XMLName field in underlying struct;
> This applies to child elements.  If you have two fields
> of type X and X has an XMLName, I think they will both
> me marshaled with the

No, marshal will report the conflict in this case, pointing out which fields are
offending.

> I think it should be
(...)
> Inner value:
> - field name/tag in the struct field
> - XMLName tag

That's one of the important bugs being fixed in this CL. We can't use the
suggested ordering unless we change the meaning of XMLName, and change the
unmarshalling side with it, and I don't think we should do that, for reasons
explained below.

Historically, and today, Unmarshal uses XMLName as validation rather than as a
hint, both in the top level and in all of the internal elements. It won't allow
anything to be unmarshalled into an XMLName field that has a tag of a different
name than the element being unmarshalled.

This behavior is intentional, and being put to good use. If I say I want an
<apple>, I don't want my code to take a <cake> silently. The marshalling side
was mistakingly allowing cakes and apples to mix up when marshalling, and thus
producing data that was impossible to unmarshal back onto to very structure that
serialized it.

Besides that aspect, it's also ambiguous, and incorrect. An <a> element in HTML
is _always_ an <a>, not a <link> not a <table>.. it's an *<a>*, independently
from the field it sits under.

If you want to marshal a structure with variable naming depending on the field
it sits under, though, it couldn't be easier: just provide a type without an
XMLName and all works as you wish. If there's an XMLName, it's for validation,
and it should match it. Explicit, and unambiguous.

Both Marshal and Unmarshal agree with the same terms in this CL, and this
cleanup means that a very large number of structures marshalled actually
unmarshal back onto the same types that serialized them.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:252: p.WriteString("<!--")
> is <!--- --> valid?  <!---->?  I can never remember the specifics.

http://www.w3.org/TR/REC-xml/#sec-comments

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go
File src/pkg/encoding/xml/read.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:235: // For now, simply ignore the field.
> I would skip the field if it has no value; if it does have 

I actually started to go into this direction, but then I've cut back and
remained with this just for fixing the explosions that otherwise happen.

This CL is already gigantic, but it was hard to reduce its size because there
was some significant refactoring needed. After I manage to get this in, I have a
few CLs in the pipeline implementing things that are improvements on it, rather
than fixups.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo.go
File src/pkg/encoding/xml/typeinfo.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:39: //fOmitEmpty
> aw, I really like omitempty...  it's not that hard, either, and now that we
have
> struct equality it's well-defined.

Both "-" and ",omitempty" are in the pipeline too. Just let me get this in and
I'll push the follow ups.

[kevlar, 2011-12-30 01:06:44]

LGTM

Comment ::= '<!--' ((Char - '-') | ('-' (Char - '-')))* '-->'

I had to stare at that for awhile before I understood the implications.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:145: // 1. XMLName field in underlying struct;
> Historically, and today, Unmarshal uses XMLName as validation rather than as a
> hint, both in the top level and in all of the internal elements. It won't
allow
> anything to be unmarshalled into an XMLName field that has a tag of a
different
> name than the element being unmarshalled.
> 
> This behavior is intentional, and being put to good use. If I say I want an
> <apple>, I don't want my code to take a <cake> silently. The marshalling side
> was mistakingly allowing cakes and apples to mix up when marshalling, and thus
> producing data that was impossible to unmarshal back onto to very structure
that
> serialized it.
> 
> Besides that aspect, it's also ambiguous, and incorrect. An <a> element in
HTML
> is _always_ an <a>, not a <link> not a <table>.. it's an *<a>*, independently
> from the field it sits under.
> 
> If you want to marshal a structure with variable naming depending on the field
> it sits under, though, it couldn't be easier: just provide a type without an
> XMLName and all works as you wish. If there's an XMLName, it's for validation,
> and it should match it. Explicit, and unambiguous.

My only problem with this is that you may not always be marshaling a structure
that you control, and someone else may have given it an XMLName that you don't
like or which doesn't work in your scenario.  I actually have never liked
XMLName in the first place, but the XML schemas that I work with haven't needed
anything like that yet.  The only upside I see to them is that it gives you the
possibility of registering types to be automatically created and stored in
interface values dynamically based on the tag/namespace name somewhere down the
line.

But I digress.  Consistency, I suppose, is the best fallback.

[niemeyer, 2011-12-30 01:23:42]

> LGTM

Cheers!

> Comment ::= '<!--' ((Char - '-') | ('-' (Char - '-')))* '-->'
>
> I had to stare at that for awhile before I understood the implications.

Yeah, it's far from obvious.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:145: // 1. XMLName field in underlying struct;
> My only problem with this is that you may not always be marshaling a structure
> that you control, and someone else may have given it an XMLName that you don't
> like or which doesn't work in your scenario.

You can easily handle that by using an anonymous field. This allows you to both
use your own XMLName choice there, and also to leave the type without an XMLName
at all, thus naming the field itself in any way desired.

That said, I'd think twice before doing that for the reasons explained before.
Using one of your examples, you'd be effectively rendering two precisely
matching types incompatible merely for not liking its name. XML is already messy
enough as it is without arbitrary element naming, so let's not help it.

> I actually have never liked XMLName in the first place,

I never liked XML in the first place. I'm working on the xml package precisely
so I don't have to look at it that often.

[adg, 2012-01-02 23:35:36]

I really like the improvements to the API in this CL. It's way better. Thanks
for the great work.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:73: //		Id        int      `xml:"id,attr"`
Please add examples of chardata, innerxml, and comment too.

In fact, it would be great to have this as a testable example in a test file, as
with $GOROOT/src/pkg/sort/example_test.go

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:162: if name == "" {
I'd bring this if block out of the surrounding if block.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:172: Escape(p, []byte(xmlns))
I wonder if there should be an EscapeString to avoid allocations here and
elsewhere in this function. Perhaps in another CL. (Add a TODO?)

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:228: switch finfo.flags & fMode {
These nested switches are becoming a bit much. Put the struct handling code into
a function?

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:244: comment = []byte(vf.String())
string comments could be handled more efficiently (but with more code). You've
definitely made the right choice to make []byte the fast path, but maybe in a
later CL we should make string comments fast too. Add a TODO?

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:249: if bytes.Index(comment, []byte("--")) >= 0
{
Set up a global:
var ddBytes = []byte("--")

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:256: p.Write([]byte{' '})
p.WriteByte(' ') ?

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go
File src/pkg/encoding/xml/read.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:31: //		Where string `xml:",attr"`
As with Marshal, can you please include the other additions in the example?

In particular, I want to see an example of ",any"

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:123: //      a struct field's XMLName tag, and the
struct field has no
s/,//

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:132: //      the XML element name is not a prefix
for the unmarshalling of
I don't understand this sentence. Can you rewrite it? Split it up? Sorry, maybe
I'm just dense.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:235: // For now, simply ignore the field.
Please make this comment a TODO with some hint as to your future plans, if you
can.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:489: for j := 0; j < len(parents); j++ {
for j := range parents {

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo.go
File src/pkg/encoding/xml/typeinfo.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:39: //fOmitEmpty
TODO :-)

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:59: for i := 0; i != n; i++ {
"i < n" is clearer, no?

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:105: func structFieldInfo(typ reflect.Type,
tinfo *typeInfo, f *reflect.StructField) (*fieldInfo, error) {
comment

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:174: } else {
Put a return above this line and drop the else block

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:248: for i, n := 0, typ.NumField(); i != n; i++
{
i < n

[rog, 2012-01-03 13:53:58]

this looks just marvellous, thanks Gustavo. nice to see someone was hacking when
i was working on the turkey. :-)

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:188: if len(str) > 0 {
if str != "" {
for consistency

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go
File src/pkg/encoding/xml/read.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:489: for j := 0; j < len(parents); j++ {
On 2012/01/02 23:35:36, adg wrote:
> for j := range parents {

or even
for j, parent := range parents {
    if parent != finfo.parents[j] {

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo.go
File src/pkg/encoding/xml/typeinfo.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:93: // Add the field in case it doesn't
conflict.
s/in case/if/ ?

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:191: if finfo.flags&fElement != 0 {
if finfo.flags&fElement == 0 {
    return finfo, nil
}

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:193: for ftyp.Kind() == reflect.Ptr {
this loop can be infinite in some cases (e.g. type A *A)

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:201: ftinfo, err := getTypeInfo(ftyp)
this can recur infinitely.
here's an example: http://paste.ubuntu.com/791615/

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:225: for typ.Kind() == reflect.Ptr {
this loop can be infinite in some cases (e.g. type A *A)

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:244: for typ.Kind() == reflect.Ptr {
can run forever (see above)

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:276: for i := 0; i < len(tinfo.fields); i++ {
for i := range tinfo.fields {

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:287: if len(oldf.parents) > len(newf.parents) {
you could use a switch instead of successive else ifs.
perhaps with a bool, e.g.

var conflicted bool
switch {
case len(oldf.parents) > len(newd.parents):
    conflicted = oldf.parents[len(newf.parents) == newf.name
case len(oldoldf.parents) < len(newf.parents):
    conflicted = newf.parents[len(oldf.parents)] == oldf.name
default:
    conflicted = newf.name == oldf.name
}
if conflicted {
    conflicts = append(conflicts, i)
}

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:326: // so drop the conflicts.
s/conflicts/conflicting fields./

[albert.strasheim, 2012-01-04 12:23:05]

Hello

On 2011/12/29 05:49:00, niemeyer wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

Just a general question: what ever happened to xml's MarshalIndent? It is still
mentioned in some of the comments and it was added as part of weekly.2011-07-07,
but it seems to have disappeared in the mean time.

Cheers

Albert

[niemeyer, 2012-01-06 17:26:59]

I'm on holiday and a bit slow this week, but I'll work on the reviews on the
plane tomorrow.

[niemeyer, 2012-01-08 20:56:56]

Thanks for the reviews.

The code is ready for another look.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:73: //		Id        int      `xml:"id,attr"`
On 2012/01/02 23:35:36, adg wrote:
> Please add examples of chardata, innerxml, and comment too.
> 
> In fact, it would be great to have this as a testable example in a test file,
as
> with $GOROOT/src/pkg/sort/example_test.go

I agree we need more examples and more documentation for how the xml package
works, but if you don't mind I'd rather see that in a separate CL so that we
don't extend the review here more than necessary.

That said, I'll be happy to fix cases where you think I'm making documentation
_worse_.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:162: if name == "" {
On 2012/01/02 23:35:36, adg wrote:
> I'd bring this if block out of the surrounding if block.

I find the current version a bit more clear/sensible in this specific case.
Re-testing only makes sense if the state changed.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:172: Escape(p, []byte(xmlns))
On 2012/01/02 23:35:36, adg wrote:
> I wonder if there should be an EscapeString to avoid allocations here and
> elsewhere in this function. Perhaps in another CL. (Add a TODO?)

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:188: if len(str) > 0 {
On 2012/01/03 13:53:58, rog wrote:
> if str != "" {
> for consistency

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:228: switch finfo.flags & fMode {
On 2012/01/02 23:35:36, adg wrote:
> These nested switches are becoming a bit much. Put the struct handling code
into
> a function?

Good idea. Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:244: comment = []byte(vf.String())
On 2012/01/02 23:35:36, adg wrote:
> string comments could be handled more efficiently (but with more code). You've
> definitely made the right choice to make []byte the fast path, but maybe in a
> later CL we should make string comments fast too. Add a TODO?

I feel a bit weird to be optimizing code that inserts comments (if speed is an
issue, I'd recommend not adding comments). OTOH, the code isn't really much more
complex, so I went ahead and did a better version. Please see what you think.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:249: if bytes.Index(comment, []byte("--")) >= 0
{
On 2012/01/02 23:35:36, adg wrote:
> Set up a global:
> var ddBytes = []byte("--")

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/marshal....
src/pkg/encoding/xml/marshal.go:256: p.Write([]byte{' '})
On 2012/01/02 23:35:36, adg wrote:
> p.WriteByte(' ') ?

Duh. Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go
File src/pkg/encoding/xml/read.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:31: //		Where string `xml:",attr"`
On 2012/01/02 23:35:36, adg wrote:
> As with Marshal, can you please include the other additions in the example?
> 
> In particular, I want to see an example of ",any"

I've improved the documentation for ",any" below, but as per the other point,
I'd prefer to have more significant refactorings of documentation and further
examples being pushed in separate CLs, to avoid holding back that big CL on
points we can easily get through afterwards.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:123: //      a struct field's XMLName tag, and the
struct field has no
On 2012/01/02 23:35:36, adg wrote:
> s/,//

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:132: //      the XML element name is not a prefix
for the unmarshalling of
On 2012/01/02 23:35:36, adg wrote:
> I don't understand this sentence. Can you rewrite it? Split it up? Sorry,
maybe
> I'm just dense.

Changed. Please see how you like it.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:235: // For now, simply ignore the field.
On 2012/01/02 23:35:36, adg wrote:
> Please make this comment a TODO with some hint as to your future plans, if you
> can.

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:489: for j := 0; j < len(parents); j++ {
On 2012/01/02 23:35:36, adg wrote:
> for j := range parents {

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo.go
File src/pkg/encoding/xml/typeinfo.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:39: //fOmitEmpty
On 2012/01/02 23:35:36, adg wrote:
> TODO :-)

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:59: for i := 0; i != n; i++ {
On 2012/01/02 23:35:36, adg wrote:
> "i < n" is clearer, no?

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:93: // Add the field in case it doesn't
conflict.
On 2012/01/03 13:53:58, rog wrote:
> s/in case/if/ ?

Why? Looks fine to me.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:105: func structFieldInfo(typ reflect.Type,
tinfo *typeInfo, f *reflect.StructField) (*fieldInfo, error) {
On 2012/01/02 23:35:36, adg wrote:
> comment

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:174: } else {
On 2012/01/02 23:35:36, adg wrote:
> Put a return above this line and drop the else block

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:191: if finfo.flags&fElement != 0 {
On 2012/01/03 13:53:58, rog wrote:
> if finfo.flags&fElement == 0 {
>     return finfo, nil
> }

The block is short and valid for elements only. I'd prefer to not break the flow
to consider everything else below as elements.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:193: for ftyp.Kind() == reflect.Ptr {
On 2012/01/03 13:53:58, rog wrote:
> this loop can be infinite in some cases (e.g. type A *A)

Is there any valid case for someone to use such a type? We could easily build a
cache of seen types, but I'd rather not if the only goal is to prevent people
from purposefully building bad types.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:201: ftinfo, err := getTypeInfo(ftyp)
On 2012/01/03 13:53:58, rog wrote:
> this can recur infinitely.
> here's an example: http://paste.ubuntu.com/791615/

Good point, it was naive indeed, thanks. I've changed this part of the code and
it's feeling a lot better now. Among other things, there's no recursion anymore.
Also added a test.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:225: for typ.Kind() == reflect.Ptr {
On 2012/01/03 13:53:58, rog wrote:
> this loop can be infinite in some cases (e.g. type A *A)

See above.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:244: for typ.Kind() == reflect.Ptr {
On 2012/01/03 13:53:58, rog wrote:
> can run forever (see above)

See above.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:248: for i, n := 0, typ.NumField(); i != n; i++
{
On 2012/01/02 23:35:36, adg wrote:
> i < n

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:276: for i := 0; i < len(tinfo.fields); i++ {
On 2012/01/03 13:53:58, rog wrote:
> for i := range tinfo.fields {

Done.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:287: if len(oldf.parents) > len(newf.parents) {
On 2012/01/03 13:53:58, rog wrote:
> you could use a switch instead of successive else ifs.
> perhaps with a bool, e.g.

The current version looks a lot more readable to me.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:326: // so drop the conflicts.
On 2012/01/03 13:53:58, rog wrote:
> s/conflicts/conflicting fields./

Done and improved further.

[rog, 2012-01-08 22:47:47]

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo.go
File src/pkg/encoding/xml/typeinfo.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:93: // Add the field in case it doesn't
conflict.
On 2012/01/08 20:56:56, niemeyer wrote:
> On 2012/01/03 13:53:58, rog wrote:
> > s/in case/if/ ?
> 
> Why? Looks fine to me.

i don't quite understand your comment which is why i made the suggestion. "in
case" usually implies guarding against something; for instance "i lock the door
in case there are thieves around". are we adding the field here to guard against
the case where it doesn't conflict? or are we adding the field only when it
doesn't conflict?

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:191: if finfo.flags&fElement != 0 {
On 2012/01/08 20:56:56, niemeyer wrote:
> On 2012/01/03 13:53:58, rog wrote:
> > if finfo.flags&fElement == 0 {
> >     return finfo, nil
> > }
> 
> The block is short and valid for elements only. I'd prefer to not break the
flow
> to consider everything else below as elements.

much better with the new shorter block.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:193: for ftyp.Kind() == reflect.Ptr {
On 2012/01/08 20:56:56, niemeyer wrote:
> On 2012/01/03 13:53:58, rog wrote:
> > this loop can be infinite in some cases (e.g. type A *A)
> 
> Is there any valid case for someone to use such a type? We could easily build
a
> cache of seen types, but I'd rather not if the only goal is to prevent people
> from purposefully building bad types.

it's possible it might be accidental. i'd prefer it if there was no type that
could cause an infinite loop at run time. that's only my opinion though -
others' might differ.
also, it's entirely possible to do the loop without needing a cache - fmt has an
example AFAIR.

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:287: if len(oldf.parents) > len(newf.parents) {
On 2012/01/08 20:56:56, niemeyer wrote:
> On 2012/01/03 13:53:58, rog wrote:
> > you could use a switch instead of successive else ifs.
> > perhaps with a bool, e.g.
> 
> The current version looks a lot more readable to me.

beg to differ - i think my version makes it obvious at a glance that a single
index is added when there's a conflict, and switch is conventional for else-if
chains with no declarations. but i don't feel too strongly. *shrug*

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

http://codereview.appspot.com/5503078/diff/16005/src/pkg/encoding/xml/typeinf...
src/pkg/encoding/xml/typeinfo.go:107: // structFieldInfo builds and returns a
fieldInfo for f.
that doesn't seem very informative to me.

perhaps something like this?

structFieldInfo returns information on the given field of the given type (typ is
used only for error generation).

http://codereview.appspot.com/5503078/diff/16005/src/pkg/encoding/xml/typeinf...
src/pkg/encoding/xml/typeinfo.go:149: return nil, fmt.Errorf("xml: invalid tag
in field %s of type %s: %q",
perhaps for consistency and conciseness, all the errors could be of the same
form as the last one in this function, for example: 
return nil, fmt.Errorf("xml: invalid tag in %s.%s: %q", typ, f.Name,
f.Tag.Get("xml"))

alternatively, change the "conflicts with name" error message to be consistent
with the others.

http://codereview.appspot.com/5503078/diff/16005/src/pkg/encoding/xml/typeinf...
src/pkg/encoding/xml/typeinfo.go:208: // in case it exists and has a valid xml
field tag, otherwise
s/in case/if/

[adg, 2012-01-08 23:25:15]

LGTM

[adg, 2012-01-08 23:25:31]

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go
File src/pkg/encoding/xml/read.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/read.go#...
src/pkg/encoding/xml/read.go:132: //      the XML element name is not a prefix
for the unmarshalling of
On 2012/01/08 20:56:56, niemeyer wrote:
> On 2012/01/02 23:35:36, adg wrote:
> > I don't understand this sentence. Can you rewrite it? Split it up? Sorry,
> maybe
> > I'm just dense.
> 
> Changed. Please see how you like it.

Much better, thanks.

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

http://codereview.appspot.com/5503078/diff/16005/src/pkg/encoding/xml/typeinf...
src/pkg/encoding/xml/typeinfo.go:208: // in case it exists and has a valid xml
field tag, otherwise
On 2012/01/08 22:47:47, rog wrote:
> s/in case/if/

+1

[niemeyer, 2012-01-08 23:33:55]

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo.go
File src/pkg/encoding/xml/typeinfo.go (right):

http://codereview.appspot.com/5503078/diff/2009/src/pkg/encoding/xml/typeinfo...
src/pkg/encoding/xml/typeinfo.go:93: // Add the field in case it doesn't
conflict.
On 2012/01/08 22:47:47, rog wrote:
> i don't quite understand your comment which is why i made the suggestion. "in

I hadn't noticed the second meaning, thanks for explaining. I've changed both
instances.

[albert.strasheim, 2012-01-09 04:49:57]

Hello

On 2012/01/08 20:56:56, niemeyer wrote:
> Thanks for the reviews.
> 
> The code is ready for another look.

There's still some comments that refer to a non-existant MarshalIndent function.
I checked the original CL and it seems MarshalIndent was never implemented. It
would be really nice to have, or maybe references to the non-existant function
could just be removed.

Cheers

Albert

[cw, 2012-01-09 05:08:07]

> There's still some comments that refer to a non-existant MarshalIndent
> function. I checked the original CL and it seems MarshalIndent was never
> implemented. It would be really nice to have, or maybe references to the
> non-existant function could just be removed.

iirc it was implemented about patch set #4 or so then taken out as the
interfaces changed

i think it's reasonable to have an implementaiton of this but clearly
until there is one the documentation should be updated

[cw, 2012-01-09 17:57:05]

(follow up from earlier, this time with a specific detail)

http://codereview.appspot.com/5503078/diff/12012/src/pkg/encoding/xml/read.go
File src/pkg/encoding/xml/read.go (left):

http://codereview.appspot.com/5503078/diff/12012/src/pkg/encoding/xml/read.go...
src/pkg/encoding/xml/read.go:178: }
TagpathError type removed

http://codereview.appspot.com/5503078/diff/12012/src/pkg/encoding/xml/read_te...
File src/pkg/encoding/xml/read_test.go (right):

http://codereview.appspot.com/5503078/diff/12012/src/pkg/encoding/xml/read_te...
src/pkg/encoding/xml/read_test.go:326: {&BadPathTestA{},
&TagPathError{reflect.TypeOf(BadPathTestA{}), "First", "items>item1", "Second",
"items"}},
TagPathError is still used here though

[gustavo_niemeyer.net, 2012-01-09 21:46:09]

> TagpathError type removed
(...)
> TagPathError is still used here though

Sorry, the last patch set (10) had a missing file somehow
(typeinfo.go). I've pushed again.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[cw, 2012-01-09 22:38:50]

LGTM

[tux21b, 2012-01-12 15:11:54]

I have just tested the new package a bit and encountered a problem:

type Problem struct {
  Amount string `xml:"foo>bar>payment"`
  Currency string `xml:"foo>bar>payment>currency,attr"`
}

The result is at the moment something like "<Problem
currency="EUR">...</Problem>" but I expected to get:

<Problem><foo><bar><payment currency="EUR">100.0</payment></bar></foo></Problem>

Regards,
Christoph

[gustavo_niemeyer.net, 2012-01-12 15:44:15]

> I have just tested the new package a bit and encountered a problem:
>
> type Problem struct {
>  Amount string `xml:"foo>bar>payment"`
>  Currency string `xml:"foo>bar>payment>currency,attr"`
> }

Thanks Christoph. Indeed this isn't supported yet and it certainly
sounds like a good idea.

That said, I'd prefer to not have it in this CL, since a few people
already went over the full (non-trivial) branch and are happy with it.
Russ told me he'd like to look at this as well, so I'll wait for fixes
coming from that review, and will let the other details discussed in
this thread to upcoming changes that others can comfortably go over
again.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[rsc, 2012-01-12 19:47:50]

LGTM

Thanks very much.

[niemeyer, 2012-01-13 10:05:26]

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

xml: major Go 1 fixup

This CL improves the xml package in the following ways:

- makes its interface match established conventions
- brings Marshal and Unmarshal closer together
- fixes a large number of bugs and adds tests
- improves speed significantly
- organizes and simplifies the code

Fixes issue 2426.
Fixes issue 2406.
Fixes issue 1989.

What follows is a detailed list of those changes.

- All matching is case sensitive without special processing
  to the field name or xml tag in an attempt to match them.
  Customize the field tag as desired to match the correct XML
  elements.

- Flags are ",flag" rather than "flag". The names "attr",
  "chardata", etc, may be used to name actual XML elements.

- Overriding of attribute names is possible with "name,attr".

- Attribute fields are marshalled properly if they have
  non-string types. Previously they were unmarshalled, but were
  ignored at marshalling time.

- Comment fields tagged with ",comment" are marshalled properly,
  rather than being marshalled as normal fields.

- The handling of the Any field has been replaced by the ",any"
  flag to avoid unexpected results when using the field name for
  other purposes, and has also been fixed to interact properly
  with name paths. Previously the feature would not function
  if any field in the type had a name path in its tag.

- Embedded struct support fixed and cleaned so it works when
  marshalling and also when using field paths deeper than one level.

- Conflict reporting on field names have been expanded to cover
  all fields. Previously it'd catch only conflicts of paths
  deeper than one level. Also interacts correctly with embedded
  structs now.

- A trailing '>' is disallowed in xml tags. It used to be
  supported for removing the ambiguity between "attr" and "attr>",
  but the marshalling support for that was broken, and it's now
  unnecessary. Use "name" instead of "name>".

- Fixed docs to point out that a XMLName doesn't have to be
  an xml.Name (e.g. a struct{} is a good fit too). The code was
  already working like that.

- Fixed asymmetry in the precedence of XML element names between
  marshalling and unmarshalling. Marshal would consider the XMLName
  of the field type before the field tag, while unmarshalling would
  do the opposite. Now both respect the tag of the XMLName field
  first, and a nice error message is provided in case an attempt
  is made to name a field with its tag in a way that would
  conflict with the underlying type's XMLName field.

- Do not marshal broken "<???>" tags when in doubt. Use the type
  name, and error out if that's not possible.

- Do not break down unmarshalling if there's an interface{} field
  in a struct.

- Significant speed boost due to caching of type metadata and
  overall allocation clean ups. The following timings reflect
  processing of the the atom test data:

  Old:

  BenchmarkMarshal           50000             48798 ns/op
  BenchmarkUnmarshal          5000            357174 ns/op

  New:

  BenchmarkMarshal          100000             19799 ns/op
  BenchmarkUnmarshal         10000            128525 ns/op

R=cw, gustavo, kevlar, adg, rogpeppe, fullung, christoph, rsc
CC=golang-dev
http://codereview.appspot.com/5503078

[knieriem, 2012-01-13 20:23:04]

Perhaps read.go needs to be slightly adjusted: It still says, matching between
XML element names and Go struct fields is case-insensitive.

--- a/src/pkg/encoding/xml/read.go
+++ b/src/pkg/encoding/xml/read.go
@@ -78,8 +78,7 @@
 // field tag.
 //
 // Because Unmarshal uses the reflect package, it can only assign
-// to exported (upper case) fields.  Unmarshal uses a case-insensitive
-// comparison to match XML element names to struct field names.
+// to exported (upper case) fields.
 //
 // Unmarshal maps an XML element to a struct using the following rules.
 // In the rules, the tag of a field refers to the value associated with the

[cw, 2012-01-13 20:30:07]

> -// to exported (upper case) fields.  Unmarshal uses a case-insensitive
> -// comparison to match XML element names to struct field names.
> +// to exported (upper case) fields.

Titlecase would be fine as well surely?

[rsc, 2012-01-13 21:11:29]

There is not supposed to be any case changes anymore.
It's just ==, like in json.

[agl2, 2012-01-14 16:53:22]

On Fri, Jan 13, 2012 at 4:11 PM, Russ Cox <rsc@golang.org> wrote:
> There is not supposed to be any case changes anymore.
> It's just ==, like in json.

The documentation still says:

"Unmarshal uses a case-insensitive comparison to match XML element
names to struct field names."

Which doesn't appear to be true and, from this thread, I think the
code is working as intended?


Cheers

AGL

[gustavo_niemeyer.net, 2012-01-14 17:06:50]

> The documentation still says:
>
> "Unmarshal uses a case-insensitive comparison to match XML element
> names to struct field names."
>
> Which doesn't appear to be true and, from this thread, I think the
> code is working as intended?

Yeah, sorry about that. I'll be pushing this and a few additional
incremental improvements/fixes in the coming days.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[agl2, 2012-01-14 21:10:04]

On Sat, Jan 14, 2012 at 12:06 PM, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> Yeah, sorry about that. I'll be pushing this and a few additional
> incremental improvements/fixes in the coming days.

Cool, no worries if you're aware. Next up:  :)

I would have expected the following program to set out.Bar.Local to
"bar", while it doesn't appear to be set at all. Previously I believe
that it was in accordance with:

"Unmarshal maps an XML element to an xml.Name by recording the element name."


Cheers

AGL

package main

import (
	"bytes"
	"encoding/xml"
	"fmt"
)

type foo struct {
	XMLName xml.Name `xml:"foo"`
	Bar     xml.Name `xml:"bar"`
}

func main() {
	input := "<foo><bar/></foo>"

	var out foo
	xml.Unmarshal(bytes.NewBufferString(input), &out)
	fmt.Printf("%#v\n", out)
}

[agl2, 2012-01-14 21:10:59]

On Sat, Jan 14, 2012 at 4:10 PM, Adam Langley <agl@google.com> wrote:
> Cool, no worries if you're aware. Next up:  :)

p.s. I'm happy to fix these if you don't have the code up in the air
at yourself.


Cheers

AGL

[gustavo_niemeyer.net, 2012-01-15 08:37:39]

> I would have expected the following program to set out.Bar.Local to
> "bar", while it doesn't appear to be set at all. Previously I believe
> that it was in accordance with:

I apologize. I see I've missed that one in the description, and also
in the documentation. I'll definitely include the documentation fix,
and can easily add the logic back and test it (it was uncovered by
tests) in case we decide to.

That said, I'd appreciate understanding the use case first, if you
don't mind. It feels like a pretty awkward feature in principle, which
is why I dropped. In your example, you're explicitly naming Bar as
"bar", so why do you care to know what bar.Local is?  It can only be
"bar".

If your need is to be able to tell if a given sub-element is present,
I suggest we add an explicit ",exists" flag or similar, that must
necessarily be a bool. It will be less redundant, and more comfortable
to use.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[agl2, 2012-01-15 18:32:24]

On Sun, Jan 15, 2012 at 3:37 AM, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> If your need is to be able to tell if a given sub-element is present,
> I suggest we add an explicit ",exists" flag or similar, that must
> necessarily be a bool. It will be less redundant, and more comfortable
> to use.

Yes, it was purely to tell whether an element was present. You can
also do that with a *string (and testing for non-nil) so it's your
call whether you think ",exists" would be generally useful.


Cheers

AGL

[rsc, 2012-01-15 18:36:48]

Let's just fill in the xml.Name always.
There's no reason _not_ to fill it in.
It's already sitting in memory taking up space.

Russ

[rsc, 2012-01-15 18:41:53]

On Sun, Jan 15, 2012 at 13:32, Adam Langley <agl@google.com> wrote:
> Yes, it was purely to tell whether an element was present. You can
> also do that with a *string (and testing for non-nil) so it's your
> call whether you think ",exists" would be generally useful.

Sorry, crossed in the mail.  I think in the current code you should
be able to define Bar []byte, which will get the inner XML of the tag,
and thus have x.Bar != nil only if the tag was present.

Russ

[agl2, 2012-01-15 18:46:54]

On Sun, Jan 15, 2012 at 1:41 PM, Russ Cox <rsc@golang.org> wrote:
> Sorry, crossed in the mail.  I think in the current code you should
> be able to define Bar []byte, which will get the inner XML of the tag,
> and thus have x.Bar != nil only if the tag was present.

It appear that, when the tag is empty, Bar []byte is still nil,
whether the tag is present or not. A *byte[] works in the same way as
a *string however.


Cheers

AGL

[rsc, 2012-01-15 18:49:14]

On Sun, Jan 15, 2012 at 13:46, Adam Langley <agl@google.com> wrote:
> It appear that, when the tag is empty, Bar []byte is still nil,
> whether the tag is present or not. A *byte[] works in the same way as
> a *string however.

Interesting, thanks.
We should make x.Bar != nil (for []byte) work too.

Russ

[gustavo_niemeyer.net, 2012-01-15 20:04:39]

> Interesting, thanks.
> We should make x.Bar != nil (for []byte) work too.

Sounds good.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[gustavo_niemeyer.net, 2012-01-15 20:05:25]

> Let's just fill in the xml.Name always.
> There's no reason _not_ to fill it in.
> It's already sitting in memory taking up space.

The point is precisely that it doesn't have to be used in this
circumstance, so it doesn't have to take up space either.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[rsc, 2012-01-15 20:25:29]

On Sun, Jan 15, 2012 at 15:05, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> The point is precisely that it doesn't have to be used in this
> circumstance, so it doesn't have to take up space either.

That's different.  If there's another way for Adam to get what
he wants, great.  But regardless of that, if xml is given a struct
with an xml.Name and decodes into it, it should fill in the name.
There is no reason not to.

Russ

[gustavo_niemeyer.net, 2012-01-15 21:47:03]

> That's different.  If there's another way for Adam to get what
> he wants, great.  But regardless of that, if xml is given a struct
> with an xml.Name and decodes into it, it should fill in the name.
> There is no reason not to.

That's true. I'll fix it and test appropriately.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[rsc, 2012-01-16 20:10:49]

On Sun, Jan 15, 2012 at 13:32, Adam Langley <agl@google.com> wrote:
> Yes, it was purely to tell whether an element was present. You can
> also do that with a *string (and testing for non-nil)

FWIW, *struct{} should work too.

[gustavo_niemeyer.net, 2012-01-16 20:18:07]

On Mon, Jan 16, 2012 at 18:10, Russ Cox <rsc@golang.org> wrote:
> On Sun, Jan 15, 2012 at 13:32, Adam Langley <agl@google.com> wrote:
>> Yes, it was purely to tell whether an element was present. You can
>> also do that with a *string (and testing for non-nil)
>
> FWIW, *struct{} should work too.

That's a curious way to spell "bool".

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[rsc, 2012-01-16 20:35:28]

On Mon, Jan 16, 2012 at 15:17, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
>> FWIW, *struct{} should work too.
>
> That's a curious way to spell "bool".

bool means that the xml body has to be the text true or false.
*struct{} is an element with no fields that are worth recording.

Russ

[gustavo_niemeyer.net, 2012-01-16 20:49:02]

>> That's a curious way to spell "bool".
>
> bool means that the xml body has to be the text true or false.
> *struct{} is an element with no fields that are worth recording.

Yes, I understand where you were coming from. It just feels a bit
artificial when thinking about the way this is going to be used as an
API, when compared to an actual bool (e.g. post.WasUpdated vs.
post.Updated != nil).

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[rsc, 2012-01-16 20:52:16]

On Mon, Jan 16, 2012 at 15:48, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> Yes, I understand where you were coming from. It just feels a bit
> artificial when thinking about the way this is going to be used as an
> API, when compared to an actual bool (e.g. post.WasUpdated vs.
> post.Updated != nil).

Are you suggesting an API change?  I wasn't.

[gustavo_niemeyer.net, 2012-01-16 22:05:11]

> Are you suggesting an API change?  I wasn't.

I already did, and was pondering out loud since I'm not sure the use
case guarantees it. Anyway, I'll focus on the growing to-do.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.