code review 4041045: token/position: added function to set file information ...

token/position: added function to set file information for a text file

godoc uses this to provide full text index position information for
non-Go files.

[gri, 2011-01-27 03:18:13]

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

I'd like you to review this change.

[rsc1, 2011-01-27 03:30:21]

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

http://codereview.appspot.com/4041045/diff/5001/src/pkg/go/token/position.go#...
src/pkg/go/token/position.go:227: // The line offsets are the offsets of the
first character of each line.
Not many people consider the empty string
at the end of a \n-terminated file to be its own line.
This comment should make it clear that SetLines does
want that, so that the file containing "abc\n" should
be passed as []int{0,4}, or maybe there is another way
to phrase it?

The fact that the zero is required seems a bit odd too.
Maybe the definition should change to be the byte
offsets of all the \n bytes in the source file, a
definition that has no ambiguity, and then this package
can deal with the subtlety of assign line numbers using
that information?

The "list of \n positions" definition doesn't have the
special case you need in SetLinesForSource.

http://codereview.appspot.com/4041045/diff/5001/src/pkg/go/token/position.go#...
src/pkg/go/token/position.go:252: // SetLinesForSource sets all line offsets for
a file given the file source.
s/Source/Bytes/
or s//Content/
?
It needn't be source code in general.

[gri, 2011-01-27 03:34:43]

On Wed, Jan 26, 2011 at 7:30 PM, <rsc@google.com> wrote:

>
>
> http://codereview.appspot.com/4041045/diff/5001/src/pkg/go/token/position.go
> File src/pkg/go/token/position.go (right):
>
>
>
http://codereview.appspot.com/4041045/diff/5001/src/pkg/go/token/position.go#...
> src/pkg/go/token/position.go:227: // The line offsets are the offsets of
>
> the first character of each line.
> Not many people consider the empty string
> at the end of a \n-terminated file to be its own line.
> This comment should make it clear that SetLines does
> want that, so that the file containing "abc\n" should
> be passed as []int{0,4}, or maybe there is another way
> to phrase it?
>
> The fact that the zero is required seems a bit odd too.
> Maybe the definition should change to be the byte
> offsets of all the \n bytes in the source file, a
> definition that has no ambiguity, and then this package
> can deal with the subtlety of assign line numbers using
> that information?
>

I've been going back and forth on this. Here's a corner case: How many lines
does an empty file have?
I agree that just recording of the offsets may be clearer. Will reconsider.

>
> The "list of \n positions" definition doesn't have the
> special case you need in SetLinesForSource.
>
>
>
http://codereview.appspot.com/4041045/diff/5001/src/pkg/go/token/position.go#...
> src/pkg/go/token/position.go:252: // SetLinesForSource sets all line
>
> offsets for a file given the file source.
> s/Source/Bytes/
> or s//Content/
> ?
> It needn't be source code in general.
>
>
> http://codereview.appspot.com/4041045/
>

[gri, 2011-01-27 04:33:59]

On Wed, Jan 26, 2011 at 7:30 PM, <rsc@google.com> wrote:

>
>
> http://codereview.appspot.com/4041045/diff/5001/src/pkg/go/token/position.go
> File src/pkg/go/token/position.go (right):
>
>
>
http://codereview.appspot.com/4041045/diff/5001/src/pkg/go/token/position.go#...
> src/pkg/go/token/position.go:227: // The line offsets are the offsets of
>
> the first character of each line.
> Not many people consider the empty string
> at the end of a \n-terminated file to be its own line.
> This comment should make it clear that SetLines does
> want that, so that the file containing "abc\n" should
> be passed as []int{0,4}, or maybe there is another way
> to phrase it?
>
> The fact that the zero is required seems a bit odd too.
> Maybe the definition should change to be the byte
> offsets of all the \n bytes in the source file, a
> definition that has no ambiguity, and then this package
> can deal with the subtlety of assign line numbers using
> that information?
>

In retrospect I believe the current encoding is actually clearer than
recording the offsets of '\n' chars:
- the start of a line is never ambiguous, it is the offset of the first
character of that line
- recording the offsets of '\n' is easy, but things get more complicated if
the newline or line terminator is '\r' '\n' or the Unicode line
terminator (U+2028)
- in those cases it's not clear which offset should be used; also it makes
it more complicated to "estimate" the column on the current line ("estimate"
because at the moment the code assumes each char is one byte)
- starting the line table with 0 may seem an odd requirement, but if a file
starts with a BOM, the offset of the first line would reasonably be 2.
- in consistency with having an empty line table for an empty line, the
encoding should only record the start offset of a new line if it is not at
the end of the file (this is already required for LineCount, but the
SetLinesFromSource doesn't do that correctly); so a file "abc\n" would have
the line table []int{0} indicating exactly one line.

- gri


>
> The "list of \n positions" definition doesn't have the
> special case you need in SetLinesForSource.
>
>
>
http://codereview.appspot.com/4041045/diff/5001/src/pkg/go/token/position.go#...
> src/pkg/go/token/position.go:252: // SetLinesForSource sets all line
>
> offsets for a file given the file source.
> s/Source/Bytes/
> or s//Content/
> ?
> It needn't be source code in general.
>
>
> http://codereview.appspot.com/4041045/
>

[gri, 2011-01-27 04:35:25]

I meant to say:

- in consistency with having an empty line table for an empty *file*, the
encoding should only record the start offset of a new line if it is not
at...

On Wed, Jan 26, 2011 at 8:33 PM, Robert Griesemer <gri@golang.org> wrote:

> - in consistency with having an empty line table for an empty line, the
> encoding should only record the start offset of a new line if it is not at
> the end of the file (this is already required for LineCount, but the
> SetLinesFromSource doesn't do that correctly); so a file "abc\n" would have
> the line table []int{0} indicating exactly one line.

[rsc, 2011-01-27 04:45:49]

> In retrospect I believe the current encoding is actually clearer than
> recording the offsets of '\n' chars:
> - the start of a line is never ambiguous, it is the offset of the first
> character of that line
> - recording the offsets of '\n' is easy, but things get more complicated if
> the newline or line terminator is '\r' '\n' or the Unicode line terminator
> (U+2028) - in those cases it's not clear which offset should be used; also
> it makes it more complicated to "estimate" the column on the current line
> ("estimate" because at the moment the code assumes each char is one byte)
> - starting the line table with 0 may seem an odd requirement, but if a file
> starts with a BOM, the offset of the first line would reasonably be 2.
> - in consistency with having an empty line table for an empty line, the
> encoding should only record the start offset of a new line if it is not at
> the end of the file (this is already required for LineCount, but the
> SetLinesFromSource doesn't do that correctly); so a file "abc\n" would have
> the line table []int{0} indicating exactly one line.

These are all very subtle rules.  Are you sure it makes more sense
to tell each client to implement them than to let each client say
where the \n positions are and have go/token implement the rules
correctly once?

It is true that the more vague concept of line admits alternate
implementations where \n does not, but I predict more clients will
want to use \n and fail to implement the rules correctly than there
will be clients that want to use something other than \n.
\r\n-ended lines (Windows) still end in \n so they work,
\r-only lines (old Mac OS) don't work but are increasingly rare,
and U+2028 lines are as far as I know a hypothetical.
It is telling that the Go compilers only accept \n.

That said, the code is fine if that's the API you choose.  LGTM.

Russ

[gri, 2011-01-27 05:38:31]

On Wed, Jan 26, 2011 at 8:45 PM, Russ Cox <rsc@golang.org> wrote:

> > In retrospect I believe the current encoding is actually clearer than
> > recording the offsets of '\n' chars:
> > - the start of a line is never ambiguous, it is the offset of the first
> > character of that line
> > - recording the offsets of '\n' is easy, but things get more complicated
> if
> > the newline or line terminator is '\r' '\n' or the Unicode line
> terminator
> > (U+2028) - in those cases it's not clear which offset should be used;
> also
> > it makes it more complicated to "estimate" the column on the current line
> > ("estimate" because at the moment the code assumes each char is one byte)
> > - starting the line table with 0 may seem an odd requirement, but if a
> file
> > starts with a BOM, the offset of the first line would reasonably be 2.
> > - in consistency with having an empty line table for an empty line, the
> > encoding should only record the start offset of a new line if it is not
> at
> > the end of the file (this is already required for LineCount, but the
> > SetLinesFromSource doesn't do that correctly); so a file "abc\n" would
> have
> > the line table []int{0} indicating exactly one line.
>
> These are all very subtle rules.  Are you sure it makes more sense
> to tell each client to implement them than to let each client say
> where the \n positions are and have go/token implement the rules
> correctly once?
>
> It is true that the more vague concept of line admits alternate
> implementations where \n does not, but I predict more clients will
> want to use \n and fail to implement the rules correctly than there
> will be clients that want to use something other than \n.
> \r\n-ended lines (Windows) still end in \n so they work,
> \r-only lines (old Mac OS) don't work but are increasingly rare,
> and U+2028 lines are as far as I know a hypothetical.
> It is telling that the Go compilers only accept \n.
>
> That said, the code is fine if that's the API you choose.  LGTM.
>

Left as is because changing it would be a change of the existing API
(AddLine already is recording the start offset of a line).
Renamed SetLinesForSource to SetLinesForContent as suggested.
Corrected invariant: The offset of the last line must be smaller than the
file size - if the "last line" is empty, it's not considered a line (as
suggested).

- gri

>
> Russ
>

[gri, 2011-01-27 05:49:18]

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

token/position: added SetLinesForContent

godoc uses this to provide full text index position information for
non-Go files.

R=rsc
CC=golang-dev
http://codereview.appspot.com/4041045

Committer: Robert Griesemer <gri@golang.org>