code review 6747043: encoding/xml: correctly escape newline, carriage return...

encoding/xml: correctly escape newline, carriage return, and tab

The generated encodings are those from
http://www.w3.org/TR/2000/WD-xml-c14n-20000119.html#charescaping

The change to the decoder ensures that we turn 
 in the
input into \r, not \n.

[iant, 2012-10-18 17:54:20]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-10-18 18:06:52]

https://codereview.appspot.com/6747043/diff/1/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):

https://codereview.appspot.com/6747043/diff/1/src/pkg/encoding/xml/xml.go#new...
src/pkg/encoding/xml/xml.go:1708: esc = []byte(fmt.Sprintf("&#x%X;", c))
I'd just add more esc_foo variables like the other cases, rather than allocating
2+ times on each newline.

[iant, 2012-10-18 19:00:36]

PTAL

https://codereview.appspot.com/6747043/diff/1/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):

https://codereview.appspot.com/6747043/diff/1/src/pkg/encoding/xml/xml.go#new...
src/pkg/encoding/xml/xml.go:1708: esc = []byte(fmt.Sprintf("&#x%X;", c))
On 2012/10/18 18:06:53, bradfitz wrote:
> I'd just add more esc_foo variables like the other cases, rather than
allocating
> 2+ times on each newline.

That is less amusing, but, OK.

[bradfitz, 2012-10-18 20:15:32]

https://codereview.appspot.com/6747043/diff/1/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):

https://codereview.appspot.com/6747043/diff/1/src/pkg/encoding/xml/xml.go#new...
src/pkg/encoding/xml/xml.go:971: } else if b0 == '\r' && b == '\n' {
b0 is the previous byte read?  that looks b1 in the loop below.  this is either
some convention I don't know, or not commented.

It's quite likely that this code is correct, and you seem to have good tests,
but I'm not confident reviewing it.  I've never read the xml code before.

[iant, 2012-10-18 20:31:51]

PTAL

https://codereview.appspot.com/6747043/diff/1/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):

https://codereview.appspot.com/6747043/diff/1/src/pkg/encoding/xml/xml.go#new...
src/pkg/encoding/xml/xml.go:971: } else if b0 == '\r' && b == '\n' {
On 2012/10/18 20:15:32, bradfitz wrote:
> b0 is the previous byte read?  that looks b1 in the loop below.  this is
either
> some convention I don't know, or not commented.

You're quite right, it should be b1.  I meant to write b1.  Fixed now.  Added a
test.

[bradfitz, 2012-10-18 20:38:13]

LGTM

On Thu, Oct 18, 2012 at 1:31 PM, <iant@golang.org> wrote:

> PTAL
>
>
>
> https://codereview.appspot.**com/6747043/diff/1/src/pkg/**
>
encoding/xml/xml.go<https://codereview.appspot.com/6747043/diff/1/src/pkg/encoding/xml/xml.go>
> File src/pkg/encoding/xml/xml.go (right):
>
> https://codereview.appspot.**com/6747043/diff/1/src/pkg/**
>
encoding/xml/xml.go#newcode971<https://codereview.appspot.com/6747043/diff/1/src/pkg/encoding/xml/xml.go#newcode971>
> src/pkg/encoding/xml/xml.go:**971: } else if b0 == '\r' && b == '\n' {
> On 2012/10/18 20:15:32, bradfitz wrote:
>
>> b0 is the previous byte read?  that looks b1 in the loop below.  this
>>
> is either
>
>> some convention I don't know, or not commented.
>>
>
> You're quite right, it should be b1.  I meant to write b1.  Fixed now.
> Added a test.
>
>
https://codereview.appspot.**com/6747043/<https://codereview.appspot.com/6747...
>

[iant, 2012-10-18 20:41:08]

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

encoding/xml: correctly escape newline, carriage return, and tab

The generated encodings are those from
http://www.w3.org/TR/2000/WD-xml-c14n-20000119.html#charescaping

The change to the decoder ensures that we turn 
 in the
input into \r, not \n.

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/6747043