mdempsky / unconvert

Remove unnecessary type conversions from Go source
BSD 3-Clause "New" or "Revised" License
377 stars 26 forks source link

column numbers and -apply don't always work with cgo #3

Closed mewmew closed 5 years ago

mewmew commented 8 years ago
$ go get azul3d.org/engine/gfx/internal/gl/2.0/gl
$ unconvert azul3d.org/engine/gfx/internal/gl/2.0/gl
panic: illegal file offset

goroutine 1 [running]:
panic(0x603c00, 0xc824298740)
    /home/u/go/src/runtime/panic.go:500 +0x189
go/token.(*File).Pos(0xc824684420, 0xd54, 0x1)
    /home/u/go/src/go/token/position.go:237 +0x88
main.print(0xc8201d7ef0, 0x43, 0xc8247ec0d0)
    /home/u/goget/src/github.com/mdempsky/unconvert/unconvert.go:119 +0x23a
main.main()
    /home/u/goget/src/github.com/mdempsky/unconvert/unconvert.go:210 +0x38f
mdempsky commented 8 years ago

Quoting from golang.org/x/tools/go/loader:

// The advantage of this approach is its fidelity to 'go build'.  The
// downside is that the token.Position.Offset for each AST node is
// incorrect, being an offset within the temporary file.  Line numbers
// should still be correct because of the //line comments.

Of course, I'm relying on token.Position.Offset to identify AST nodes within a file. Doh.

I guess I need to switch to using (Line, Column) instead.

mdempsky commented 8 years ago

See @tamird's tests at https://github.com/mdempsky/unconvert/pull/12#issuecomment-237026407

TL;DR: We really only have accurate line number info for cgo-processed files, not even column number.

To implement it correctly, we'll need some sort of matching heuristic to correlate intraline position info between the cgo-using files before and after cgo processing.

mdempsky commented 5 years ago

I think this could be fixed by having upstream use /*line*/ comments so that we have accurate column numbers after cgo mutations.

mdempsky commented 5 years ago

Looks like we'll have that in Go 1.12 actually: golang/go#26745.

mdempsky commented 5 years ago

Confirmed that this will be fixed for Go 1.12, including support for -apply.