code review 2936041: token/position: implemented Pos

token/position: implemented Pos

A pos value represents a file-set specific, accurate
source position value. It is 8x smaller in size than
the corresponding Position value (4 bytes vs 32 bytes).

Using Pos values instead of Position values in AST
saves approx. 25MBytes of memory when running godoc
on the current repository.

This CL introduces the Pos, File, and FileSet data
types; it does not affect existing code. Another
(pending CL) will make the change to all dependent
source files.

Missing: tests

[rsc1, 2010-11-05 18:55:37]

http://codereview.appspot.com/2936041/diff/5001/src/pkg/go/token/position.go
File src/pkg/go/token/position.go (right):

http://codereview.appspot.com/2936041/diff/5001/src/pkg/go/token/position.go#...
src/pkg/go/token/position.go:61: // NoPos may be used when no offset information
is available;
NoOffset?

http://codereview.appspot.com/2936041/diff/5001/src/pkg/go/token/position.go#...
src/pkg/go/token/position.go:139: // Name returns number of lines in file f.
s/Name/LineCount/
s/returns/returns the/

[gri, 2010-11-12 22:55:44]

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

I'd like you to review this change.

[r, 2010-11-12 23:22:06]

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go
File src/pkg/go/token/position.go (right):

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:16: // Position describes an arbitrary source
position.
including the file, line, and column location.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:61: // Pos describes a FileSet-specific source
position.
Pos is a compact encoding of a source position within a file set. It can be
converted into a Position for a more convenient, but much larger,
representation.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:74: // to the respective file set before the file
implied by q.
i would have expected the comparison to be lexicographic based on file name, but
i doubt it's important  it's not even clear to me that the comparability of
Pos's is important

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:106: // Position returns the Position value for p.
perhaps
Position converts a Pos in the fileset into a general Position
?

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:139: panic("illegal line offset")
panic seems wrong here, since it could be caused by user error and is not an
internal failure.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:147: // A File has a name, size, and a line offset
table.
s/size/a size/
or
s/ a line/line/

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:189: panic("illegal line offset")
ditto about panic

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:205: panic("illegal line offset")
ditto etc.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:278: // A FileSet describes a set of files.
s/describes/represents/
s/files/source files/

[gri, 2010-11-12 23:45:15]

PTAL

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go
File src/pkg/go/token/position.go (right):

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:16: // Position describes an arbitrary source
position.
On 2010/11/12 23:22:06, r wrote:
> including the file, line, and column location.

Done.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:61: // Pos describes a FileSet-specific source
position.
On 2010/11/12 23:22:06, r wrote:
> Pos is a compact encoding of a source position within a file set. It can be
> converted into a Position for a more convenient, but much larger,
> representation.

Done.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:74: // to the respective file set before the file
implied by q.
On 2010/11/12 23:22:06, r wrote:
> i would have expected the comparison to be lexicographic based on file name,
but
> i doubt it's important  it's not even clear to me that the comparability of
> Pos's is important

It's easy to do that by adding a specific compare function. Being able to
compare positions directly is useful for (binary) searching positions, and to
determine if something comes before something else (e.g. when positioning
comments).

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:106: // Position returns the Position value for p.
On 2010/11/12 23:22:06, r wrote:
> perhaps
> Position converts a Pos in the fileset into a general Position
> ?

Done.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:139: panic("illegal line offset")
On 2010/11/12 23:22:06, r wrote:
> panic seems wrong here, since it could be caused by user error and is not an
> internal failure.

Done.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:147: // A File has a name, size, and a line offset
table.
On 2010/11/12 23:22:06, r wrote:
> s/size/a size/
> or
> s/ a line/line/

Done.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:189: panic("illegal line offset")
On 2010/11/12 23:22:06, r wrote:
> ditto about panic

Done.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:205: panic("illegal line offset")
On 2010/11/12 23:22:06, r wrote:
> ditto etc.

Done.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:278: // A FileSet describes a set of files.
On 2010/11/12 23:22:06, r wrote:
> s/describes/represents/
> s/files/source files/

Done.

http://codereview.appspot.com/2936041/diff/15002/src/pkg/go/token/position.go...
src/pkg/go/token/position.go:278: // A FileSet describes a set of files.
On 2010/11/12 23:22:06, r wrote:
> s/describes/represents/
> s/files/source files/

Done.

[r, 2010-11-12 23:48:24]

LGTM

[gri, 2010-11-13 00:39:36]

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

token/position: implemented Pos

A pos value represents a file-set specific, accurate
source position value. It is 8x smaller in size than
the corresponding Position value (4 bytes vs 32 bytes).

Using Pos values instead of Position values in AST
saves approx. 25MBytes of memory when running godoc
on the current repository.

This CL introduces the Pos, File, and FileSet data
types; it does not affect existing code. Another
(pending CL) will make the change to all dependent
source files.

Missing: tests

R=r
CC=golang-dev, rsc
http://codereview.appspot.com/2936041

Committer: Robert Griesemer <gri@golang.org>