code review 5279044: html: improve parsing of comments and "bogus comments"

html: improve parsing of comments and "bogus comments"

[andybalholm, 2011-10-14 03:55:33]

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

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

[andybalholm, 2011-10-14 03:56:45]

This CL was written simultaneously with 5262044 and will require some adjustment
in order to merge correctly.

[nigeltao, 2011-10-14 04:02:28]

Can you take the "malformed tag #5-8" tests out and revert the change
from "z.tt, z.err = ErrorToken, os.NewError("html: TODO: handle
malformed tags")" to "z.nextText()"? I think that that will minimize
the upcoming merge conflict.

[nigeltao, 2011-10-14 04:03:49]

http://codereview.appspot.com/5279044/diff/4001/src/pkg/html/token_test.go
File src/pkg/html/token_test.go (right):

http://codereview.appspot.com/5279044/diff/4001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:234: // DOCTYPE tests.
I'd move the DOCTYPE and XML tests up to be just after the comment tests.

[andybalholm, 2011-10-14 04:26:45]

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

Please take another look.

[andybalholm, 2011-10-14 04:27:59]

This revision of the CL applies cleanly after CL 5262044.

[andybalholm, 2011-10-14 04:29:41]

http://codereview.appspot.com/5279044/diff/4001/src/pkg/html/token_test.go
File src/pkg/html/token_test.go (right):

http://codereview.appspot.com/5279044/diff/4001/src/pkg/html/token_test.go#ne...
src/pkg/html/token_test.go:234: // DOCTYPE tests.
On 2011/10/14 04:03:49, nigeltao wrote:
> I'd move the DOCTYPE and XML tests up to be just after the comment tests.

Done.

[nigeltao, 2011-10-14 06:10:34]

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go
File src/pkg/html/token.go (right):

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode231
src/pkg/html/token.go:231: //   z.raw.start + 4 <= z.raw.end.
Join this line with the previous one, now that there's room.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode243
src/pkg/html/token.go:243: dashCount++
I'd put a "continue" after this, and move all the "dashCount = 0" lines from the
end of the other case blocks to after the "switch c" block.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode248
src/pkg/html/token.go:248: z.data.end = z.data.start // For comments like <!-->,
with no data.
I normally put the comment above, not to the right. Same at line 326.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode250
src/pkg/html/token.go:250: z.tt = CommentToken
This line should no longer be necessary.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode261
src/pkg/html/token.go:261: if c == '>' {
It may be easier to track a "var exclamation bool" instead of copy/pasting the
"c == '>'" case.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode277
src/pkg/html/token.go:277: // Pre-condition: z.err == nil && z.raw.end points at
the first byte to be ignored.
80 chars for doc comments, or s/byte to be ignored/comment byte/.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode305
src/pkg/html/token.go:305: break
I don't think "break" is the right thing to do. It's not written down (but
should be), but the idea is that you don't call z.readByte if z.err != nil. By
breaking here, you call readByte at line 315.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode338
src/pkg/html/token.go:338: z.skipWhiteSpace()
Again, don't call z.readByte later if z.skipWhiteSpace results in an error.
Lines 338-340 should be:
----
z.tt = DoctypeToken
if z.skipWhiteSpace(); z.err != nil {
  z.data.start = z.raw.end
  z.data.end = z.raw.end
  return
}
z.data.start = z.raw.end
----

Also, please add a test that is "<!DOCtype".

[andybalholm, 2011-10-14 15:02:56]

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

Please take another look.

[andybalholm, 2011-10-14 15:03:07]

PTAL

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go
File src/pkg/html/token.go (right):

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode231
src/pkg/html/token.go:231: //   z.raw.start + 4 <= z.raw.end.
On 2011/10/14 06:10:34, nigeltao wrote:
> Join this line with the previous one, now that there's room.

Done.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode243
src/pkg/html/token.go:243: dashCount++
On 2011/10/14 06:10:34, nigeltao wrote:
> I'd put a "continue" after this, and move all the "dashCount = 0" lines from
the
> end of the other case blocks to after the "switch c" block.

Done.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode248
src/pkg/html/token.go:248: z.data.end = z.data.start // For comments like <!-->,
with no data.
On 2011/10/14 06:10:34, nigeltao wrote:
> I normally put the comment above, not to the right. Same at line 326.

Done.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode250
src/pkg/html/token.go:250: z.tt = CommentToken
On 2011/10/14 06:10:34, nigeltao wrote:
> This line should no longer be necessary.

Done.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode261
src/pkg/html/token.go:261: if c == '>' {
On 2011/10/14 06:10:34, nigeltao wrote:
> It may be easier to track a "var exclamation bool" instead of copy/pasting the
> "c == '>'" case.
I think that would make the control flow harder to follow. I'd have to label the
loop and add a break statement. A goto would almost be cleaner. But this is Go,
so I can use defer. 

I like the semantics of defer here. It says, "No matter what, make sure that you
don't return data with a negative length." But it probably slows things down a
little. If you think that's a problem, I can go back to having the if statement
in two places.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode277
src/pkg/html/token.go:277: // Pre-condition: z.err == nil && z.raw.end points at
the first byte to be ignored.
On 2011/10/14 06:10:34, nigeltao wrote:
> 80 chars for doc comments, or s/byte to be ignored/comment byte/.

Done.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode305
src/pkg/html/token.go:305: break
On 2011/10/14 06:10:34, nigeltao wrote:
> I don't think "break" is the right thing to do. It's not written down (but
> should be), but the idea is that you don't call z.readByte if z.err != nil. By
> breaking here, you call readByte at line 315.

Done.

http://codereview.appspot.com/5279044/diff/9001/src/pkg/html/token.go#newcode338
src/pkg/html/token.go:338: z.skipWhiteSpace()
On 2011/10/14 06:10:34, nigeltao wrote:
> Again, don't call z.readByte later if z.skipWhiteSpace results in an error.
> Lines 338-340 should be:
> ----
> z.tt = DoctypeToken
> if z.skipWhiteSpace(); z.err != nil {
>   z.data.start = z.raw.end
>   z.data.end = z.raw.end
>   return
> }
> z.data.start = z.raw.end
> ----
> 
> Also, please add a test that is "<!DOCtype".

Done. (with slightly different order to avoid repetition)

[nigeltao, 2011-10-15 00:44:34]

LGTM.

There are a couple of little nits to fix, but I'll do them when I submit this.

[nigeltao, 2011-10-15 01:22:14]

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

html: improve parsing of comments and "bogus comments"

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

Committer: Nigel Tao <nigeltao@golang.org>