code review 86990044: go/scanner: interpret //line directives sans filename s...

go/scanner: interpret //line directives sans filename sensibly

A //line directive without a filename now denotes the same
filename as the previous line (as in C).
Previously it denoted the file's directory (!).

Fixes issue 7765

[adonovan, 2014-04-11 17:36:42]

Hello gri@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[gri, 2014-04-14 19:38:02]

LGTM

https://codereview.appspot.com/86990044/diff/40001/src/pkg/go/scanner/scanner.go
File src/pkg/go/scanner/scanner.go (right):

https://codereview.appspot.com/86990044/diff/40001/src/pkg/go/scanner/scanner...
src/pkg/go/scanner/scanner.go:152: // valid //line filename:line comment.
leave away . at end - this is not a sentence anyway

https://codereview.appspot.com/86990044/diff/40001/src/pkg/go/scanner/scanner...
src/pkg/go/scanner/scanner.go:153: filename :=
strings.TrimSpace(string(text[len(prefix):i]))
string(bytes.TrimSpace(text[...]))

less work for string() and no need to import strings

https://codereview.appspot.com/86990044/diff/40001/src/pkg/go/scanner/scanner...
src/pkg/go/scanner/scanner.go:155: // assume same file as previous line
s/as/as for/

https://codereview.appspot.com/86990044/diff/40001/src/pkg/go/scanner/scanner...
File src/pkg/go/scanner/scanner_test.go (right):

https://codereview.appspot.com/86990044/diff/40001/src/pkg/go/scanner/scanner...
src/pkg/go/scanner/scanner_test.go:498: {"\n//line :123\n  line1",
"dir/File2.go", 123},
- add an entry where there's more than just one blank between //line and :
- test also with tabs

[adonovan, 2014-04-16 18:51:35]

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

go/scanner: interpret //line directives sans filename sensibly

A //line directive without a filename now denotes the same
filename as the previous line (as in C).
Previously it denoted the file's directory (!).

Fixes issue 7765

LGTM=gri
R=gri
CC=golang-codereviews
https://codereview.appspot.com/86990044

[adonovan, 2014-04-16 18:51:54]

All done.

https://codereview.appspot.com/86990044/diff/40001/src/pkg/go/scanner/scanner.go
File src/pkg/go/scanner/scanner.go (right):

https://codereview.appspot.com/86990044/diff/40001/src/pkg/go/scanner/scanner...
src/pkg/go/scanner/scanner.go:152: // valid //line filename:line comment.
On 2014/04/14 19:38:02, gri wrote:
> leave away . at end - this is not a sentence anyway

Done.

https://codereview.appspot.com/86990044/diff/40001/src/pkg/go/scanner/scanner...
src/pkg/go/scanner/scanner.go:153: filename :=
strings.TrimSpace(string(text[len(prefix):i]))
On 2014/04/14 19:38:02, gri wrote:
> string(bytes.TrimSpace(text[...]))
> 
> less work for string() and no need to import strings

Done.

https://codereview.appspot.com/86990044/diff/40001/src/pkg/go/scanner/scanner...
src/pkg/go/scanner/scanner.go:155: // assume same file as previous line
On 2014/04/14 19:38:02, gri wrote:
> s/as/as for/

Done.

[rsc, 2014-04-16 18:54:44]

Who is generating these buggy line directives?

[adonovan, 2014-04-16 18:59:16]

On 16 April 2014 14:54, <rsc@golang.org> wrote:

> Who is generating these buggy line directives?
>

godoc emits them in some cases, e.g. preceding each field of a struct type
declaration in _cgo_types.go.

(I asked gri offline for his preferred interpretation of them, and he said
consistency with what C does.)

[gobot, 2014-04-16 19:04:26]

This CL appears to have broken the windows-amd64-race builder.
See http://build.golang.org/log/8e5f1c394f40fef3c9e04ff4b5c27f175184a83b

[rsc, 2014-04-16 19:04:54]

When we defined the //line originally, an explicit goal of mine was to be
simpler than C. The only allowed format is //line file:line, where you
write both. This is documented in 'godoc cmd/gc' (amazingly). The
implication is that if you write "//line :10" then it means file="",
line=10. That's dumb, but also the point: it can be implemented with dumb
code. There's only one form, are no magic rules that everyone has to know
and implement.

Tools should always generate //line file:line.
Tools should only parse //line file:line.

Generating or allowing alternate forms in one tool or another eventually
means you have to allow them in all tools. The rules in C are needlessly
complex for such a relatively unimportant feature, and we're trying to be
less complex.

[adonovan, 2014-04-16 19:13:03]

Ok; sorry I didn't cc: you for the review.

Rather than roll back this change, I'll modify it since the previously
implemented behaviour was still not in conformance with your expressed
wishes:
//line :1 was parsed not as file="", but as file=$(cwd).


On 16 April 2014 15:04, Russ Cox <rsc@golang.org> wrote:

> When we defined the //line originally, an explicit goal of mine was to be
> simpler than C. The only allowed format is //line file:line, where you
> write both. This is documented in 'godoc cmd/gc' (amazingly). The
> implication is that if you write "//line :10" then it means file="",
> line=10. That's dumb, but also the point: it can be implemented with dumb
> code. There's only one form, are no magic rules that everyone has to know
> and implement.
>
> Tools should always generate //line file:line.
> Tools should only parse //line file:line.
>
> Generating or allowing alternate forms in one tool or another eventually
> means you have to allow them in all tools. The rules in C are needlessly
> complex for such a relatively unimportant feature, and we're trying to be
> less complex.
>
>

[rsc, 2014-04-16 19:15:44]

sgtm, thanks.