code review 4216050: xml: permit nested directives.

xml: permit nested directives.
    Modify <! case in RawToken so that nested <! ...> directives are
    accumulated rather than terminating the opening directive at the
    first >. Keep track of nesting depth, +1 for each <, -1 for each
    >, but not counting < or > inside single-quoted or double-quoted
    strings.
    Fixes issue 1549.

[eh, 2011-02-24 21:16:40]

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

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

[niemeyer, 2011-02-25 02:55:06]

Thanks for the change.

There's a small problem in the handling of quotes:

http://codereview.appspot.com/4216050/diff/1001/src/pkg/xml/xml.go
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/4216050/diff/1001/src/pkg/xml/xml.go#newcode560
src/pkg/xml/xml.go:560: } else if b == '\'' || b == '"' {
Add a test for a string such as "'".  The center
quote will switch inquote improperly.

It should look something like this:

switch {
case b == inquote:
    inquote = 0
case inquote != 0:
    // nothing
case b == '\'' || b == '"':
    inquote = b
case b == '>':
    depth -= 1
case b == '<':
    depth += 1
}

[eh, 2011-02-25 07:38:22]

http://codereview.appspot.com/4216050/diff/1001/src/pkg/xml/xml.go
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/4216050/diff/1001/src/pkg/xml/xml.go#newcode560
src/pkg/xml/xml.go:560: } else if b == '\'' || b == '"' {
On 2011/02/25 02:55:07, niemeyer wrote:
> Add a test for a string such as "'".  The center
> quote will switch inquote improperly.
> 
> It should look something like this:
> 
> switch {
> case b == inquote:
>     inquote = 0
> case inquote != 0:
>     // nothing
> case b == '\'' || b == '"':
>     inquote = b
> case b == '>':
>     depth -= 1
> case b == '<':
>     depth += 1
> }

Done, but still as if-chain -- would a switch be preferred style here?

[Also, should the copyright comment be changed to reflect a 2011 update?]

[eh, 2011-02-25 09:42:47]

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

Please take another look.

[eh, 2011-02-25 09:43:11]

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

Please take another look.

[eh, 2011-02-25 09:46:13]

On 25 February 2011 09:43,  <ehog.hedge@googlemail.com> wrote:
> Hello golang-dev@googlegroups.com, niemeyer (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4216050/
>

Sorry for the duplicate posting. I got confused about exactly
what needed to be redone after I got a

  warning: gofmt needs to format these files (run hg gofmt):

message.

Chris

-- 
Chris "allusive" Dollin

[niemeyer, 2011-02-25 13:02:42]

> Done, but still as if-chain -- would a switch be
> preferred style here?

Yeah, I think it is more idiomatic for these cases.
Wasn't for the new case introduced, you could even
switch on b.

> [Also, should the copyright comment be changed to reflect a 2011 update?]

Not use about what's the practice for updating those (and avoiding
conflicts while doing so), but since there are no other updated
files at this point in the tree, I'd say leave it alone for now.

[rsc, 2011-02-25 15:23:49]

> [Also, should the copyright comment be changed to reflect a 2011
> update?]

no.  we will revisit in 2080.

[eh, 2011-02-25 16:23:04]

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

Please take another look.

[niemeyer, 2011-02-25 17:34:16]

LGTM

[eh, 2011-02-25 18:43:07]

On 25 February 2011 17:34,  <n13m3y3r@gmail.com> wrote:
> LGTM
>
> http://codereview.appspot.com/4216050/
>

So, as I understand it, since I'm not a submitter, someone-perhaps-you
will now import & submit (if still OK) the code.

I e-signed a CLA yesterday, so you should be able to find that
for any necessary magic, this being my first time and all.

Chris

-- 
Chris "allusive" Dollin

[niemeyer, 2011-02-25 18:47:18]

> So, as I understand it, since I'm not a submitter, someone-perhaps-you
> will now import & submit (if still OK) the code.

Right, except I'm not a submitter either.  Someone
will look at this and comment/approve/submit.

> I e-signed a CLA yesterday, so you should be able to find that
> for any necessary magic, this being my first time and all.

Welcome, and thanks!

[rsc, 2011-02-25 18:51:04]

http://codereview.appspot.com/4216050/diff/17001/src/pkg/xml/xml.go
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/4216050/diff/17001/src/pkg/xml/xml.go#newcode569
src/pkg/xml/xml.go:569: depth -= 1
depth--

http://codereview.appspot.com/4216050/diff/17001/src/pkg/xml/xml.go#newcode572
src/pkg/xml/xml.go:572: depth += 1
depth++

[eh, 2011-02-26 10:12:58]

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

Please take another look.

[eh, 2011-02-26 10:13:33]

http://codereview.appspot.com/4216050/diff/1001/src/pkg/xml/xml.go
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/4216050/diff/1001/src/pkg/xml/xml.go#newcode560
src/pkg/xml/xml.go:560: } else if b == '\'' || b == '"' {
On 2011/02/25 02:55:07, niemeyer wrote:
> Add a test for a string such as "'".  The center
> quote will switch inquote improperly.
> 
> It should look something like this:
> 
> switch {
> case b == inquote:
>     inquote = 0
> case inquote != 0:
>     // nothing
> case b == '\'' || b == '"':
>     inquote = b
> case b == '>':
>     depth -= 1
> case b == '<':
>     depth += 1
> }

Done.

http://codereview.appspot.com/4216050/diff/17001/src/pkg/xml/xml.go#newcode569
src/pkg/xml/xml.go:569: }
On 2011/02/25 18:51:04, rsc wrote:
> depth--

Done.

http://codereview.appspot.com/4216050/diff/17001/src/pkg/xml/xml.go#newcode572
src/pkg/xml/xml.go:572: p.ungetc(b)
On 2011/02/25 18:51:04, rsc wrote:
> depth++

Done.

[rsc, 2011-02-28 15:09:31]

LGTM

[rsc, 2011-02-28 19:09:10]

*** Submitted as 768498ada484 ***

xml: permit nested directives

Return <!DOCTYPE ...> with nested directives as one big token.

Fixes issue 1549.

R=niemeyer, rsc
CC=golang-dev
http://codereview.appspot.com/4216050

Committer: Russ Cox <rsc@golang.org>

[peterGo, 2011-02-28 19:18:34]

http://codereview.appspot.com/4216050/diff/19001/src/pkg/xml/xml.go
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/4216050/diff/19001/src/pkg/xml/xml.go#newcode561
src/pkg/xml/xml.go:561: 
To be consistent with the rest of xml.go and elsewhere, when there are only a
few statements in the switch statement case clauses, the blank lines separating
the case clauses are unnecessary.