code review 7106045: xml: Support fields not of type []byte when marshaling ...

xml: Support fields not of type []byte when marshaling ",chardata"

Fixes issue 4506.

[vegacom, 2013-01-14 13:45:25]

Hello golang-dev@googlegroups.com,

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

[vegacom, 2013-01-15 13:39:30]

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

Please take another look.

[vegacom, 2013-01-15 13:40:29]

The first patch lacked support for time.Time.

It's added in this latest version.

[rsc, 2013-01-18 22:13:06]

Looks good, thanks.

https://codereview.appspot.com/7106045/diff/6004/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

https://codereview.appspot.com/7106045/diff/6004/src/pkg/encoding/xml/marshal...
src/pkg/encoding/xml/marshal.go:288: Escape(p,
[]byte(strconv.FormatFloat(vf.Float(), 'g', -1, 64)))
This should be two different cases. Float32 and Float64 format differently.

[remyoudompheng, 2013-01-18 22:14:45]

https://codereview.appspot.com/7106045/diff/6004/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

https://codereview.appspot.com/7106045/diff/6004/src/pkg/encoding/xml/marshal...
src/pkg/encoding/xml/marshal.go:283: case reflect.Int, reflect.Int8,
reflect.Int16, reflect.Int32, reflect.Int64:
you should be able to save an allocation by declaring a local small buffer and
using AppendInt/AppendUint/AppendFloat

[vegacom, 2013-01-19 02:43:42]

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

Please take another look.

[vegacom, 2013-01-19 02:44:09]

Thanks for the review.

is it in the scope of this CL to fix marshalSimple?

case reflect.Float32, reflect.Float64:
 p.WriteString(strconv.FormatFloat(val.Float(), 'g', -1, 64))

instead of 64, it should be val.Type().Bits()

https://codereview.appspot.com/7106045/diff/6004/src/pkg/encoding/xml/marshal.go
File src/pkg/encoding/xml/marshal.go (right):

https://codereview.appspot.com/7106045/diff/6004/src/pkg/encoding/xml/marshal...
src/pkg/encoding/xml/marshal.go:283: case reflect.Int, reflect.Int8,
reflect.Int16, reflect.Int32, reflect.Int64:
On 2013/01/18 22:14:46, remyoudompheng wrote:
> you should be able to save an allocation by declaring a local small buffer and
> using AppendInt/AppendUint/AppendFloat

Done.

https://codereview.appspot.com/7106045/diff/6004/src/pkg/encoding/xml/marshal...
src/pkg/encoding/xml/marshal.go:288: Escape(p,
[]byte(strconv.FormatFloat(vf.Float(), 'g', -1, 64)))
On 2013/01/18 22:13:06, rsc wrote:
> This should be two different cases. Float32 and Float64 format differently.

Done.

[rsc, 2013-01-22 22:45:47]

LGTM

I'll submit this now since it's ready but feel free to send in a separate CL
fixing the marshalSimple float32 case. Thanks.

[rsc, 2013-01-22 22:46:31]

Actually, it looks like you need to complete a CLA as described at
golang.org/doc/contribute.html#copyright. Thanks.

[vegacom, 2013-01-22 23:01:43]

Done

[rsc, 2013-01-23 03:13:46]

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

xml: Support fields not of type []byte when marshaling ",chardata"

Fixes issue 4506.

R=rsc, remyoudompheng
CC=golang-dev
https://codereview.appspot.com/7106045

Committer: Russ Cox <rsc@golang.org>