code review 6260046: exp/html: Convert \r and \r\n to \n when tokenizing

exp/html: Convert \r and \r\n to \n when tokenizing

Also escape "\r" as "
" when rendering HTML.

Pass 2 additional tests.

[andybalholm, 2012-05-29 04:27:28]

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

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

[nigeltao, 2012-05-29 06:45:29]

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/escape.go
File src/pkg/exp/html/escape.go (right):

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/escape.go#ne...
src/pkg/exp/html/escape.go:195: const escapedChars = `&'<>"` + "\r"
Make the RHS string "&'<>\"\r"

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/token.go
File src/pkg/exp/html/token.go (right):

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/token.go#new...
src/pkg/exp/html/token.go:701: func convertNewlines(s []byte) []byte {
This func needs its own test.

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/token.go#new...
src/pkg/exp/html/token.go:703: if c == '\r' {
There would be less indentation if you re-order your code:

if c != '\r' {
  continue
}
src := i + 1
if src >= len(s) || s[src] != '\n' {
  s[i] = '\n'
  continue
}
dst := i
for src < len(s) {
  // etc
}
return s[:dst]

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/token.go#new...
src/pkg/exp/html/token.go:709: if src < len(s)-1 && s[src+1] == '\n' {
Change "src < len(s)-1" to "src+1 < len(s)".

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/token.go#new...
src/pkg/exp/html/token.go:739: 
Delete the blank line.

[andybalholm, 2012-05-29 15:13:51]

PTAL.

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/escape.go
File src/pkg/exp/html/escape.go (right):

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/escape.go#ne...
src/pkg/exp/html/escape.go:195: const escapedChars = `&'<>"` + "\r"
On 2012/05/29 06:45:29, nigeltao wrote:
> Make the RHS string "&'<>\"\r"

Done.

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/token.go
File src/pkg/exp/html/token.go (right):

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/token.go#new...
src/pkg/exp/html/token.go:701: func convertNewlines(s []byte) []byte {
On 2012/05/29 06:45:29, nigeltao wrote:
> This func needs its own test.

Done.

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/token.go#new...
src/pkg/exp/html/token.go:703: if c == '\r' {
On 2012/05/29 06:45:29, nigeltao wrote:
> There would be less indentation if you re-order your code:
> 
> if c != '\r' {
>   continue
> }
> src := i + 1
> if src >= len(s) || s[src] != '\n' {
>   s[i] = '\n'
>   continue
> }
> dst := i
> for src < len(s) {
>   // etc
> }
> return s[:dst]

Done.

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/token.go#new...
src/pkg/exp/html/token.go:709: if src < len(s)-1 && s[src+1] == '\n' {
On 2012/05/29 06:45:29, nigeltao wrote:
> Change "src < len(s)-1" to "src+1 < len(s)".

Done.

http://codereview.appspot.com/6260046/diff/5001/src/pkg/exp/html/token.go#new...
src/pkg/exp/html/token.go:739: 
On 2012/05/29 06:45:29, nigeltao wrote:
> Delete the blank line.

Done.

[nigeltao, 2012-05-30 01:23:50]

http://codereview.appspot.com/6260046/diff/8001/src/pkg/exp/html/token_test.go
File src/pkg/exp/html/token_test.go (right):

http://codereview.appspot.com/6260046/diff/8001/src/pkg/exp/html/token_test.g...
src/pkg/exp/html/token_test.go:593: var newlineTests = []tokenTest{
Please also test these inputs:
""
"\n"
"\n\r"
"\r"
"\r\n"
"\r\n\n"
"\r\n\r"
"\r\n\r\n"
"\r\r"
"\r\r\n"
"\r\r\n\n"
"\r\r\r\n"
"\r \n"  // Note the space.
"xyz"

I wouldn't bother with a description for each test. The input (formatted as %q)
should be sufficient:

var testCases := []struct{
  in, want string
}{
  "",
  "\n",
  // etc.
}
for _, tc := range testCases {
  if got := string(convertNewLines([]byte(tc.in))); got != test.want {
    t.Errorf("input %q: got %q, want %q", tc.in, got, tc.want)
  }
}

[nigeltao, 2012-05-30 03:02:52]

Also, please "hg sync" before you next "hg upload", as I've just submitted an
exp/html change.

[andybalholm, 2012-05-30 04:09:36]

PTAL.

http://codereview.appspot.com/6260046/diff/8001/src/pkg/exp/html/token_test.go
File src/pkg/exp/html/token_test.go (right):

http://codereview.appspot.com/6260046/diff/8001/src/pkg/exp/html/token_test.g...
src/pkg/exp/html/token_test.go:593: var newlineTests = []tokenTest{
On 2012/05/30 01:23:50, nigeltao wrote:
> Please also test these inputs:
> ""
> "\n"
> "\n\r"
> "\r"
> "\r\n"
> "\r\n\n"
> "\r\n\r"
> "\r\n\r\n"
> "\r\r"
> "\r\r\n"
> "\r\r\n\n"
> "\r\r\r\n"
> "\r \n"  // Note the space.
> "xyz"

Done

> var testCases := []struct{
>   in, want string
> }

I used a map, because it makes the code look cleaner (to me, at least). I don't
think the indeterminate iteration order should be a problem.

[nigeltao, 2012-05-30 05:47:00]

LGTM.

I'll fix the test placement, and submit.

http://codereview.appspot.com/6260046/diff/8001/src/pkg/exp/html/token_test.go
File src/pkg/exp/html/token_test.go (right):

http://codereview.appspot.com/6260046/diff/8001/src/pkg/exp/html/token_test.g...
src/pkg/exp/html/token_test.go:593: var newlineTests = []tokenTest{
On 2012/05/30 04:09:36, andybalholm wrote:
> I used a map, because it makes the code look cleaner (to me, at least). I
don't
> think the indeterminate iteration order should be a problem.

Nice.

http://codereview.appspot.com/6260046/diff/8002/src/pkg/exp/html/token_test.go
File src/pkg/exp/html/token_test.go (right):

http://codereview.appspot.com/6260046/diff/8002/src/pkg/exp/html/token_test.g...
src/pkg/exp/html/token_test.go:658: func TestConvertNewlines(t *testing.T) {
Move the tests up above the benchmarks.

[nigeltao, 2012-05-30 05:50:26]

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

exp/html: Convert \r and \r\n to \n when tokenizing

Also escape "\r" as "
" when rendering HTML.

Pass 2 additional tests.

R=nigeltao
CC=golang-dev
http://codereview.appspot.com/6260046

Committer: Nigel Tao <nigeltao@golang.org>

[nigeltao, 2012-05-30 05:53:27]

For the record, the before/after benchmarks:

BenchmarkParser      500       4626146 ns/op      16.90 MB/s
BenchmarkRawLevelTokenizer      2000        894498 ns/op      87.38 MB/s
BenchmarkLowLevelTokenizer      2000       1118470 ns/op      69.88 MB/s
BenchmarkHighLevelTokenizer     1000       2006670 ns/op      38.95 MB/s

BenchmarkParser      500       4839040 ns/op      16.15 MB/s
BenchmarkRawLevelTokenizer      2000        904150 ns/op      86.45 MB/s
BenchmarkLowLevelTokenizer      2000       1260548 ns/op      62.01 MB/s
BenchmarkHighLevelTokenizer     1000       2175799 ns/op      35.92 MB/s

Performance is not the priority at the moment; correctness is. This is
just a data point.