Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
LGTM
*** Submitted as http://code.google.com/p/go/source/detail?r=59d5bc43be1f *** go/scanner: return literal as string instead of []byte Removed many string conversions in dependent code. Runs all tests. No change to gofmt output. R=r CC=golang-dev http://codereview.appspot.com/4291070 Committer: Robert Griesemer <gri@golang.org>
LGTM http://codereview.appspot.com/4291070/diff/5016/src/cmd/govet/govet.go File src/cmd/govet/govet.go (right): http://codereview.appspot.com/4291070/diff/5016/src/cmd/govet/govet.go#newcod... src/cmd/govet/govet.go:259: if strings.Index(lit.Value, "%") < 0 { if strings.Contains(lit.Value, "%") { since you're there http://codereview.appspot.com/4291070/diff/5016/src/cmd/govet/govet.go#newcod... src/cmd/govet/govet.go:341: if strings.Index(lit.Value, "%") >= 0 { strings.Contains http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/printer/nodes.go File src/pkg/go/printer/nodes.go (right): http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/printer/nodes.go#n... src/pkg/go/printer/nodes.go:530: if e.Op == token.QUO { worth a comment, as it were if e.Op == token.QUO { // "*/" http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/printer/printer.go File src/pkg/go/printer/printer.go (right): http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/printer/printer.go... src/pkg/go/printer/printer.go:130: // Escapes strings pass through tabwriter unchanged. (Note that s/Escapes/Escaped/ http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/printer/printer.go... src/pkg/go/printer/printer.go:139: return p.litbuf.String() I haven't examined this in detail, but it appears that this function's result gets passed to writeItem, which converts it to []byte in order to call Write. Maybe escape should return []byte to avoid the two conversions (and allocations)? That assumes the result of each call to p.escape gets used before the next call to p.escape, but that appears to be the case. http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/scanner/scanner.go File src/pkg/go/scanner/scanner.go (right): http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/scanner/scanner.go... src/pkg/go/scanner/scanner.go:713: // TODO(gri): The scanner API should change such that the literal string I think the more efficient implementation can be done without restricting the API: var lit string if tok == token.IDENT || tok == token.NUMBER { lit = string(S.src[offs:S.offset]) } else { lit = tok.String() }
http://codereview.appspot.com/4291070/diff/5016/src/cmd/govet/govet.go File src/cmd/govet/govet.go (right): http://codereview.appspot.com/4291070/diff/5016/src/cmd/govet/govet.go#newcod... src/cmd/govet/govet.go:259: if strings.Index(lit.Value, "%") < 0 { On 2011/03/29 03:52:20, rsc wrote: > if strings.Contains(lit.Value, "%") { > > since you're there Done. http://codereview.appspot.com/4291070/diff/5016/src/cmd/govet/govet.go#newcod... src/cmd/govet/govet.go:341: if strings.Index(lit.Value, "%") >= 0 { On 2011/03/29 03:52:20, rsc wrote: > strings.Contains Done. http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/printer/nodes.go File src/pkg/go/printer/nodes.go (right): http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/printer/nodes.go#n... src/pkg/go/printer/nodes.go:530: if e.Op == token.QUO { On 2011/03/29 03:52:20, rsc wrote: > worth a comment, as it were > > if e.Op == token.QUO { // "*/" Done. http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/printer/printer.go File src/pkg/go/printer/printer.go (right): http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/printer/printer.go... src/pkg/go/printer/printer.go:130: // Escapes strings pass through tabwriter unchanged. (Note that On 2011/03/29 03:52:20, rsc wrote: > s/Escapes/Escaped/ Done. http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/printer/printer.go... src/pkg/go/printer/printer.go:139: return p.litbuf.String() On 2011/03/29 03:52:20, rsc wrote: > I haven't examined this in detail, but it appears that > this function's result gets passed to writeItem, which > converts it to []byte in order to call Write. > Maybe escape should return []byte to avoid the two > conversions (and allocations)? > > That assumes the result of each call to p.escape gets > used before the next call to p.escape, but that appears > to be the case. Correct. I want to push the strings a bit further but at the end I need to have a []byte to write. There's a TODO. It's not great, but it's better than what was there before, I think. http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/scanner/scanner.go File src/pkg/go/scanner/scanner.go (right): http://codereview.appspot.com/4291070/diff/5016/src/pkg/go/scanner/scanner.go... src/pkg/go/scanner/scanner.go:713: // TODO(gri): The scanner API should change such that the literal string On 2011/03/29 03:52:20, rsc wrote: > I think the more efficient implementation can be done > without restricting the API: > > var lit string > if tok == token.IDENT || tok == token.NUMBER { > lit = string(S.src[offs:S.offset]) > } else { > lit = tok.String() > } + taking care of AllowIllegalChars (albeit now that we have the non-Go specific scanner, I want to remove that flag eventually). Also, token.NUMBER is really INT, FLOAT, IMAG, CHAR, and there is STRING. I still prefer to not compute the token string in non-literal cases. It's never really used. Leaving as is for now.
