code review 6099043: net/http: lex cleanup

net/http: lex cleanup

[pascal, 2012-04-20 20:19:29]

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/

[rsc, 2012-04-20 20:25:36]

http://codereview.appspot.com/6099043/diff/5001/src/pkg/net/http/lex.go
File src/pkg/net/http/lex.go (right):

http://codereview.appspot.com/6099043/diff/5001/src/pkg/net/http/lex.go#newcode9
src/pkg/net/http/lex.go:9: const tokenTable = "                                
! #$%&'  *+ -. 0123456789       ABCDEFGHIJKLMNOPQRSTUVWXYZ  
^_`abcdefghijklmnopqrstuvwxyz | ~"
This is magic that must be maintained: it's not a string of characters but a
lookup table.

Can you do the cleanup without using strings as lookup tables?
It would be fine to have a string with all the token characters 
and use ContainsRune(tokens, rune(c))

[bradfitz, 2012-04-20 20:29:49]

On Fri, Apr 20, 2012 at 1:25 PM, <rsc@golang.org> wrote:

>
> http://codereview.appspot.com/**6099043/diff/5001/src/pkg/net/**
>
http/lex.go<http://codereview.appspot.com/6099043/diff/5001/src/pkg/net/http/lex.go>
> File src/pkg/net/http/lex.go (right):
>
> http://codereview.appspot.com/**6099043/diff/5001/src/pkg/net/**
>
http/lex.go#newcode9<http://codereview.appspot.com/6099043/diff/5001/src/pkg/net/http/lex.go#newcode9>
> src/pkg/net/http/lex.go:9: const tokenTable = "
>       ! #$%&'  *+ -. 0123456789       ABCDEFGHIJKLMNOPQRSTUVWXYZ
> ^_`abcdefghijklmnopqrstuvwxyz | ~"
> This is magic that must be maintained: it's not a string of characters
> but a lookup table.
>

I agree this table could be listed better (not using a string), but the
spec about what a token means isn't a very fluid.  I think a table is fine.


> Can you do the cleanup without using strings as lookup tables?
> It would be fine to have a string with all the token characters
> and use ContainsRune(tokens, rune(c))
>

That can't be inlined and requires a scan over the table.

[rsc, 2012-04-20 20:31:20]

On Fri, Apr 20, 2012 at 16:29, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I agree this table could be listed better (not using a string), but the
> spec about what a token means isn't a very fluid.  I think a table is fine.

I don't object to a table.
I object to a table indexed by character value.

Russ

[rsc, 2012-04-20 20:33:59]

On Fri, Apr 20, 2012 at 16:31, Russ Cox <rsc@golang.org> wrote:
> I don't object to a table.
> I object to a table indexed by character value.

to be clear, it is the implicit indexing that is the problem.
obviously these two are different but which is correct?

 const tokenTable = "     ! #$%&'   *+..."
 const tokenTable = "      ! #$%&'  *+..."

if a table is necessary for speed (and i think that's unclear)
it can be written in ways that make the indices clearer.

var isToken = [256]bool{
   '!': true,
   '#': true,
   ...
}

[bradfitz, 2012-04-20 20:35:23]

On Fri, Apr 20, 2012 at 1:33 PM, Russ Cox <rsc@golang.org> wrote:

> On Fri, Apr 20, 2012 at 16:31, Russ Cox <rsc@golang.org> wrote:
> > I don't object to a table.
> > I object to a table indexed by character value.
>
> to be clear, it is the implicit indexing that is the problem.
> obviously these two are different but which is correct?
>
>  const tokenTable = "     ! #$%&'   *+..."
>  const tokenTable = "      ! #$%&'  *+..."
>
> if a table is necessary for speed (and i think that's unclear)
> it can be written in ways that make the indices clearer.
>
> var isToken = [256]bool{
>   '!': true,
>   '#': true,
>   ...
> }
>

Yeah, that's what I meant by "listed better".  But that's still "indexed by
character value".

[rsc, 2012-04-20 20:38:55]

On Fri, Apr 20, 2012 at 16:35, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Yeah, that's what I meant by "listed better".  But that's still "indexed by
> character value".

but not implicitly

[pascal, 2012-04-20 21:49:21]

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

Please take another look.

[rsc, 2012-04-20 21:51:04]

http://codereview.appspot.com/6099043/diff/10001/src/pkg/net/http/cookie.go
File src/pkg/net/http/cookie.go (right):

http://codereview.appspot.com/6099043/diff/10001/src/pkg/net/http/cookie.go#n...
src/pkg/net/http/cookie.go:261: isNotToken := func(r rune) bool { return
!isToken(r) }
Please make this a top-level function.  Right now this code will allocate each
time it is run.

http://codereview.appspot.com/6099043/diff/10001/src/pkg/net/http/lex.go
File src/pkg/net/http/lex.go (right):

http://codereview.appspot.com/6099043/diff/10001/src/pkg/net/http/lex.go#newc...
src/pkg/net/http/lex.go:10: '!': true, '#': true, '$': true, '%': true,
Please put 1 entry per line.
It is more lines, but then they can be sorted,
maintained, and skimmed more easily.

[pascal, 2012-04-20 22:02:38]

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

Please take another look.

[rsc, 2012-04-23 13:47:29]

http://codereview.appspot.com/6099043/diff/6003/src/pkg/net/http/lex.go
File src/pkg/net/http/lex.go (right):

http://codereview.appspot.com/6099043/diff/6003/src/pkg/net/http/lex.go#newco...
src/pkg/net/http/lex.go:96: return i < len(isTokenTable) && !isTokenTable[i]
isToken and isNotToken both return false when i >= len(isTokenTable).
That seems wrong.
Maybe make this

return !isToken(r)

and then they can't be out of sync.

[pascal, 2012-04-23 16:35:34]

Hi Russ.

> That seems wrong.

According to the spec it's correct but confusing none the less.

> Maybe make this
> 
> return !isToken(r)

Done.

> http://codereview.appspot.com/6099043/

[rsc, 2012-04-23 17:13:53]

LGTM

Leaving for Brad.

[bradfitz, 2012-04-23 17:25:53]

LGTM

On Mon, Apr 23, 2012 at 10:13 AM, <rsc@golang.org> wrote:

> LGTM
>
> Leaving for Brad.
>
>
>
http://codereview.appspot.com/**6099043/<http://codereview.appspot.com/6099043/>
>

[bradfitz, 2012-04-23 17:27:28]

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

net/http: lex cleanup

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

Committer: Brad Fitzpatrick <bradfitz@golang.org>