Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(94)

Issue 7438051: code review 7438051: encoding/xml: Marshal/Escape allows invalid characters

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 1 month ago by osaingre
Modified:
11 years ago
Reviewers:
rsc
CC:
rsc, dfc, r, volker.dobler, golang-dev
Visibility:
Public.

Description

encoding/xml: Marshal/Escape allows invalid characters Fixes issue 4235.

Patch Set 1 #

Patch Set 2 : diff -r 2f04e02f33c0 https://code.google.com/p/go #

Patch Set 3 : diff -r 2f04e02f33c0 https://code.google.com/p/go #

Total comments: 14

Patch Set 4 : diff -r a1ce0637ab2b https://code.google.com/p/go #

Patch Set 5 : diff -r 149360268947 https://code.google.com/p/go #

Total comments: 2

Patch Set 6 : diff -r 149360268947 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -5 lines) Patch
M src/pkg/encoding/xml/xml.go View 1 2 3 4 5 3 chunks +11 lines, -4 lines 0 comments Download
M src/pkg/encoding/xml/xml_test.go View 1 2 3 2 chunks +17 lines, -1 line 0 comments Download

Messages

Total messages: 19
osaingre
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
11 years, 1 month ago (2013-03-01 01:56:47 UTC) #1
dfc
On 2013/03/01 01:56:47, osaingre wrote: > Hello mailto:golang-dev@googlegroups.com (cc: mailto:golang-dev@googlegroups.com), > > I'd like you ...
11 years, 1 month ago (2013-03-02 02:01:59 UTC) #2
osaingre
On 2013/03/02 02:01:59, dfc wrote: > On 2013/03/01 01:56:47, osaingre wrote: > > Hello mailto:golang-dev@googlegroups.com ...
11 years ago (2013-03-03 19:56:24 UTC) #3
osaingre
On 2013/03/03 19:56:24, osaingre wrote: > On 2013/03/02 02:01:59, dfc wrote: > > On 2013/03/01 ...
11 years ago (2013-03-08 10:07:25 UTC) #4
r
This is an inefficient implementation. It adds another scan over the data, plus some allocation ...
11 years ago (2013-03-08 21:03:34 UTC) #5
osaingre
On 2013/03/08 21:03:34, r wrote: > This is an inefficient implementation. It adds another scan ...
11 years ago (2013-03-08 22:03:00 UTC) #6
volker.dobler
https://codereview.appspot.com/7438051/diff/5001/src/pkg/encoding/xml/xml.go File src/pkg/encoding/xml/xml.go (right): https://codereview.appspot.com/7438051/diff/5001/src/pkg/encoding/xml/xml.go#newcode1725 src/pkg/encoding/xml/xml.go:1725: func isInvalidChar(r rune) bool { This function is redundant. ...
11 years ago (2013-03-11 12:10:55 UTC) #7
rsc
This CL can be made simple enough that I would like to see this fix ...
11 years ago (2013-03-12 19:51:37 UTC) #8
rsc
https://codereview.appspot.com/7438051/diff/5001/src/pkg/encoding/xml/xml.go File src/pkg/encoding/xml/xml.go (right): https://codereview.appspot.com/7438051/diff/5001/src/pkg/encoding/xml/xml.go#newcode1782 src/pkg/encoding/xml/xml.go:1782: default: Actually, you can just skip the whole bytes.Map ...
11 years ago (2013-03-12 19:54:07 UTC) #9
osaingre
Hello, I made the suggested changes. Benchmark is as follows: benchmark old ns/op new ns/op ...
11 years ago (2013-03-13 01:07:28 UTC) #10
r
https://codereview.appspot.com/7438051/diff/5001/src/pkg/encoding/xml/xml.go File src/pkg/encoding/xml/xml.go (right): https://codereview.appspot.com/7438051/diff/5001/src/pkg/encoding/xml/xml.go#newcode1782 src/pkg/encoding/xml/xml.go:1782: default: s is a slice of bytes, so you ...
11 years ago (2013-03-13 01:27:04 UTC) #11
osaingre
On 2013/03/13 01:27:04, r wrote: > https://codereview.appspot.com/7438051/diff/5001/src/pkg/encoding/xml/xml.go > File src/pkg/encoding/xml/xml.go (right): > > https://codereview.appspot.com/7438051/diff/5001/src/pkg/encoding/xml/xml.go#newcode1782 > ...
11 years ago (2013-03-13 08:34:51 UTC) #12
osaingre
I removed the call to bytes.Map(). PTAL. benchmark old ns/op new ns/op delta BenchmarkMarshal 25833 ...
11 years ago (2013-03-13 21:59:36 UTC) #13
rsc
https://codereview.appspot.com/7438051/diff/25001/src/pkg/encoding/xml/xml.go File src/pkg/encoding/xml/xml.go (right): https://codereview.appspot.com/7438051/diff/25001/src/pkg/encoding/xml/xml.go#newcode1737 src/pkg/encoding/xml/xml.go:1737: esc_fffd := []byte("\uFFFD") Please put this into the var ...
11 years ago (2013-03-13 22:02:30 UTC) #14
osaingre
https://codereview.appspot.com/7438051/diff/25001/src/pkg/encoding/xml/xml.go File src/pkg/encoding/xml/xml.go (right): https://codereview.appspot.com/7438051/diff/25001/src/pkg/encoding/xml/xml.go#newcode1737 src/pkg/encoding/xml/xml.go:1737: esc_fffd := []byte("\uFFFD") On 2013/03/13 22:02:30, rsc wrote: > ...
11 years ago (2013-03-13 22:10:00 UTC) #15
rsc
Did that affect the benchmark?
11 years ago (2013-03-13 22:13:35 UTC) #16
osaingre
On 2013/03/13 22:13:35, rsc wrote: > Did that affect the benchmark? Yes indeed. After moving ...
11 years ago (2013-03-13 22:23:53 UTC) #17
rsc
LGTM Thanks for iterating with us.
11 years ago (2013-03-14 03:24:26 UTC) #18
rsc
11 years ago (2013-03-14 03:26:09 UTC) #19
*** Submitted as https://code.google.com/p/go/source/detail?r=62c96dd0ddc3 ***

encoding/xml: rewrite invalid code points to U+FFFD in Marshal, Escape

Fixes issue 4235.

R=rsc, dave, r, dr.volker.dobler
CC=golang-dev
https://codereview.appspot.com/7438051

Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b