cmd/compile: character position vs line comments #22662
Comments
|
One thing to consider is the impact of gofmt. For instance, it could make sense to define /*line file:line:col */ as the position of the first Go token following it (rather than the byte immediately following). Since the /*line ... */ annotations are (likely) produced by a tool, and that tool is using positions of existing tokens, it might make those positions more stable in the presence of minor gofmt formatting changes (extra white space on the same line). |
|
Change https://golang.org/cl/86037 mentions this issue: |
griesemer
added
the
early-in-cycle
|
@rsc At least as specified, there is a fundamental problem on Windows because filenames may contain a drive letter followed by a (from test/fixedbugs/issue18149.go) the pragma processor assumes that we have a filename For line directives with an invalid line number specification one could simply assume that the line number section is the filename; with such a work-around the above example would work. But file names may be numbers, and then the work-around falls flat: Is this a line directive with filename Without optional column specification we don't have this problem: Currently, a line directive must have a There's a several ways out of the dilemma: 1) introduce some form of escape mechanism for any Here is an alternative format that doesn't have the problem. Borrowing from C's line directive, we could use the forms: where But at least on OS X, file names can have blanks in them, so some suitable escape mechanism will still be needed. |
|
In C's If the quotes are there, we apply the usual string constant escape handling. |
|
@ianlancetaylor That's not backward-compatible with what we have now: Existing directive texts with two I think any solution requires some kind of format change. |
|
One could require the filename to always be a string constant (or absent) if a column is specified. Otherwise it's the existing format (with the filename optionally being a string constant). |
R=go1.11 This implements parsing of /*line file:line*/ and /*line file:line:col*/ directives and also extends the optional column format to regular //line directives, per #22662. For a line directive to be recognized, its comment text must start with the prefix "line " which is followed by one of the following: :line :line:col filename:line filename:line:col with at least one : present. The line and col values must be unsigned decimal integers; everything before is considered part of the filename. Valid line directives are: //line :123 //line :123:8 //line foo.go:123 //line C:foo.go:123 (filename is "C:foo.go") //line C:foo.go:123:8 (filename is "C:foo.go") /*line ::123*/ (filename is ":") No matter the comment format, at the moment all directives act as if they were in //line comments, and column information is ignored. To be addressed in subsequent CLs. For #22662. Change-Id: I1a2dc54bacc94bc6cdedc5229ee13278971f314e Reviewed-on: https://go-review.googlesource.com/86037 Reviewed-by: Matthew Dempsky <mdempsky@google.com>
|
Change https://golang.org/cl/94515 mentions this issue: |
|
Change https://golang.org/cl/96476 mentions this issue: |
|
Change https://golang.org/cl/96535 mentions this issue: |
For dependency reasons, the data structure implementing source positions in the compiler is in cmd/internal/src. It contains highly compiler specific details (e.g. inlining index). This change introduces a parallel but simpler position representation, defined in the syntax package, which removes that package's dependency on cmd/internal/src, and also removes the need to deal with certain filename-specific operations (defined by the needs of the compiler) in the syntax package. As a result, the syntax package becomes again a compiler- independent, stand-alone package that at some point might replace (or augment) the existing top-level go/* syntax-related packages. Additionally, line directives that update column numbers are now correctly tracked through the syntax package, with additional tests added. (The respective changes also need to be made in cmd/internal/src; i.e., the compiler accepts but still ignores column numbers in line directives.) This change comes at the cost of a new position translation step, but that step is cheap because it only needs to do real work if the position base changed (i.e., if there is a new file, or new line directive). There is no noticeable impact on overall compiler performance measured with `compilebench -count 5 -alloc`: name old time/op new time/op delta Template 220ms ± 8% 228ms ±18% ~ (p=0.548 n=5+5) Unicode 119ms ±11% 113ms ± 5% ~ (p=0.056 n=5+5) GoTypes 684ms ± 6% 677ms ± 3% ~ (p=0.841 n=5+5) Compiler 3.19s ± 7% 3.01s ± 1% ~ (p=0.095 n=5+5) SSA 7.92s ± 8% 7.79s ± 1% ~ (p=0.690 n=5+5) Flate 141ms ± 7% 139ms ± 4% ~ (p=0.548 n=5+5) GoParser 173ms ±12% 171ms ± 4% ~ (p=1.000 n=5+5) Reflect 417ms ± 5% 411ms ± 3% ~ (p=0.548 n=5+5) Tar 205ms ± 5% 198ms ± 2% ~ (p=0.690 n=5+5) XML 232ms ± 4% 229ms ± 4% ~ (p=0.690 n=5+5) StdCmd 28.7s ± 5% 28.2s ± 2% ~ (p=0.421 n=5+5) name old user-time/op new user-time/op delta Template 269ms ± 4% 265ms ± 5% ~ (p=0.421 n=5+5) Unicode 153ms ± 7% 149ms ± 3% ~ (p=0.841 n=5+5) GoTypes 850ms ± 7% 862ms ± 4% ~ (p=0.841 n=5+5) Compiler 4.01s ± 5% 3.86s ± 0% ~ (p=0.190 n=5+4) SSA 10.9s ± 4% 10.8s ± 2% ~ (p=0.548 n=5+5) Flate 166ms ± 7% 167ms ± 6% ~ (p=1.000 n=5+5) GoParser 204ms ± 8% 206ms ± 7% ~ (p=0.841 n=5+5) Reflect 514ms ± 5% 508ms ± 4% ~ (p=0.548 n=5+5) Tar 245ms ± 6% 244ms ± 3% ~ (p=0.690 n=5+5) XML 280ms ± 4% 278ms ± 4% ~ (p=0.841 n=5+5) name old alloc/op new alloc/op delta Template 37.9MB ± 0% 37.9MB ± 0% ~ (p=0.841 n=5+5) Unicode 28.8MB ± 0% 28.8MB ± 0% ~ (p=0.841 n=5+5) GoTypes 113MB ± 0% 113MB ± 0% ~ (p=0.151 n=5+5) Compiler 468MB ± 0% 468MB ± 0% -0.01% (p=0.032 n=5+5) SSA 1.50GB ± 0% 1.50GB ± 0% ~ (p=0.548 n=5+5) Flate 24.4MB ± 0% 24.4MB ± 0% ~ (p=1.000 n=5+5) GoParser 30.7MB ± 0% 30.7MB ± 0% ~ (p=1.000 n=5+5) Reflect 76.5MB ± 0% 76.5MB ± 0% ~ (p=0.548 n=5+5) Tar 38.9MB ± 0% 38.9MB ± 0% ~ (p=0.222 n=5+5) XML 41.6MB ± 0% 41.6MB ± 0% ~ (p=0.548 n=5+5) name old allocs/op new allocs/op delta Template 382k ± 0% 382k ± 0% +0.01% (p=0.008 n=5+5) Unicode 343k ± 0% 343k ± 0% ~ (p=0.841 n=5+5) GoTypes 1.19M ± 0% 1.19M ± 0% +0.01% (p=0.008 n=5+5) Compiler 4.53M ± 0% 4.53M ± 0% +0.03% (p=0.008 n=5+5) SSA 12.4M ± 0% 12.4M ± 0% +0.00% (p=0.008 n=5+5) Flate 235k ± 0% 235k ± 0% ~ (p=0.079 n=5+5) GoParser 318k ± 0% 318k ± 0% ~ (p=0.730 n=5+5) Reflect 978k ± 0% 978k ± 0% ~ (p=1.000 n=5+5) Tar 393k ± 0% 393k ± 0% ~ (p=0.056 n=5+5) XML 405k ± 0% 405k ± 0% ~ (p=0.548 n=5+5) name old text-bytes new text-bytes delta HelloSize 672kB ± 0% 672kB ± 0% ~ (all equal) CmdGoSize 7.12MB ± 0% 7.12MB ± 0% ~ (all equal) name old data-bytes new data-bytes delta HelloSize 133kB ± 0% 133kB ± 0% ~ (all equal) CmdGoSize 390kB ± 0% 390kB ± 0% ~ (all equal) name old exe-bytes new exe-bytes delta HelloSize 1.07MB ± 0% 1.07MB ± 0% ~ (all equal) CmdGoSize 11.2MB ± 0% 11.2MB ± 0% ~ (all equal) Passes toolstash compare. For #22662. Change-Id: I19edb53dd9675af57f7122cb7dba2a6d8bdcc3da Reviewed-on: https://go-review.googlesource.com/94515 Reviewed-by: Matthew Dempsky <mdempsky@google.com>
For line directives which have a line and a column number, an omitted filename means that the filename has not changed (per the issue below). For line directives w/o a column number, an omitted filename means the empty filename (to preserve the existing behavior). For #22662. Change-Id: I32cd9037550485da5445a34bb104706eccce1df1 Reviewed-on: https://go-review.googlesource.com/96476 Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Extend cmd/internal/src.PosBase to track column information, and adjust the meaning of the PosBase position to mean the position at which the PosBase's relative (line, col) position starts (rather than indicating the position of the //line directive). Because this semantic change is made in the compiler's noder, it doesn't affect the logic of src.PosBase, only its test setup (where PosBases are constructed with corrected incomming positions). In short, src.PosBase now matches syntax.PosBase with respect to the semantics of src.PosBase.pos. For #22662. Change-Id: I5b1451cb88fff3f149920c2eec08b6167955ce27 Reviewed-on: https://go-review.googlesource.com/96535 Reviewed-by: Matthew Dempsky <mdempsky@google.com>
griesemer
changed the title
|
Created #24143 to track the non-compiler aspects of this issue. |
|
Change https://golang.org/cl/97375 mentions this issue: |
If the compiler reads //line xxx:10, it understands the next line in the source file to be attributed to file xxx line 10, but it also disables reporting of character position information, because often generated code does not preserve character positions when converting from the original file to the generated code. But sometimes a generator can preserve that information, and it would be nice for our tools to be able to present it.
I suggest that we allow
/*line xxx:10:20*/, where thexxxmay be omitted if the file name has not changed since the last comment, to declare that the very next byte after that comment is :10:20, and character position should be tracked and reported until the next line comment. Similarly,//line xxx:10:20would say that the next line begins at character position 20. It's important to have a /* */ comment form in order to be able to introduce a line annotation without introducing an implicit semicolon.With this and some pending CLs I have for cmd/cover and cmd/cgo, the errors reported by the compiler would be exactly as precise when using coverage as when not using coverage, and the same for cgo.
Also, because the character position would be precise, using cgo-preprocessed sources in source code refactoring tools would no longer prevent those tools from modifying the original sources.
This is orthogonal to #16623 (passing unmodified cgo sources to the compiler and perhaps also go/types), since it would still be very helpful for cover and other converters (we can't put them all in the compiler and go/types).
The text was updated successfully, but these errors were encountered: