Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(14236)

Issue 4291070: code review 4291070: go/scanner: return literal as string instead of []byte (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years ago by gri
Modified:
12 years, 9 months ago
Reviewers:
rsc
CC:
r, golang-dev
Visibility:
Public.

Description

go/scanner: return literal as string instead of []byte Removed unnecessary string conversions in dependent code. Runs all tests.

Patch Set 1 #

Patch Set 2 : diff -r 6659c68c1d45 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 56b847e640f3 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 56b847e640f3 https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r f75a02fd83be https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r f75a02fd83be https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r f75a02fd83be https://go.googlecode.com/hg/ #

Patch Set 8 : diff -r e3615176d528 https://go.googlecode.com/hg/ #

Patch Set 9 : diff -r ea196694b740 https://go.googlecode.com/hg/ #

Patch Set 10 : diff -r 9eeaf3d9215c https://go.googlecode.com/hg/ #

Patch Set 11 : diff -r 4073ecdfc054 https://go.googlecode.com/hg/ #

Patch Set 12 : diff -r 4073ecdfc054 https://go.googlecode.com/hg/ #

Patch Set 13 : diff -r 4073ecdfc054 https://go.googlecode.com/hg/ #

Patch Set 14 : diff -r 4073ecdfc054 https://go.googlecode.com/hg/ #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+130 lines, -133 lines) Patch
M src/cmd/cgo/gcc.go View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/godoc/spec.go View 1 2 4 3 chunks +3 lines, -3 lines 0 comments Download
M src/cmd/govet/govet.go View 1 2 4 8 chunks +7 lines, -10 lines 4 comments Download
M src/pkg/ebnf/parser.go View 1 2 4 4 chunks +4 lines, -4 lines 0 comments Download
M src/pkg/exp/datafmt/parser.go View 1 2 4 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/exp/ogle/cmd.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/go/ast/ast.go View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/go/ast/filter.go View 1 2 4 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/go/doc/comment.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/go/doc/doc.go View 1 2 4 3 chunks +4 lines, -4 lines 0 comments Download
M src/pkg/go/parser/parser.go View 1 2 3 4 5 6 7 8 10 chunks +16 lines, -24 lines 0 comments Download
M src/pkg/go/printer/nodes.go View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 2 comments Download
M src/pkg/go/printer/printer.go View 1 2 3 4 5 6 7 8 9 10 25 chunks +63 lines, -56 lines 4 comments Download
M src/pkg/go/scanner/scanner.go View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +12 lines, -10 lines 2 comments Download
M src/pkg/go/scanner/scanner_test.go View 1 2 4 5 chunks +7 lines, -8 lines 0 comments Download
M src/pkg/go/typechecker/typechecker_test.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 5
gri
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
13 years ago (2011-03-28 22:30:45 UTC) #1
r
LGTM
13 years ago (2011-03-28 23:42:08 UTC) #2
gri
*** Submitted as http://code.google.com/p/go/source/detail?r=59d5bc43be1f *** go/scanner: return literal as string instead of []byte Removed many ...
13 years ago (2011-03-28 23:44:44 UTC) #3
rsc
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#newcode259 src/cmd/govet/govet.go:259: if strings.Index(lit.Value, "%") < 0 { if strings.Contains(lit.Value, ...
13 years ago (2011-03-29 03:52:19 UTC) #4
gri
13 years ago (2011-03-29 04:44:34 UTC) #5
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b