Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
mewmew opened this issue Mar 8, 2016 · 5 comments
Closed

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

mewmew opened this issue Mar 8, 2016 · 5 comments
Assignees
@mdempsky

Comments

@mewmew
Copy link

mewmew commented Mar 8, 2016 

$ 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 mdempsky self-assigned this Mar 8, 2016
@mdempsky
Copy link
Owner

mdempsky commented Mar 8, 2016

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.

@tamird tamird mentioned this issue Jul 29, 2016
Merged
mdempsky pushed a commit that referenced this issue Aug 2, 2016
@tamird @mdempsky
unconvert: don't panic on source that uses cgo
6cd281c 
Package loader parses the cgo-processed source,
so offsets and column numbers don't match the original
source.  This commit changes the internal code to track
the exploded token.Position values instead of raw integer
offsets, so that unconvert can at least report line numbers
of unnecessary conversions in cgo-using source files.

Updates #3.
@mdempsky mdempsky changed the title panic: illegal file offset (possibly related to cgo) column numbers and -apply don't always work with cgo Aug 2, 2016
@mdempsky
Copy link
Owner

mdempsky commented Aug 2, 2016

See @tamird's tests at #12 (comment)

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 mdempsky reopened this Aug 2, 2016
@mdempsky mdempsky mentioned this issue Nov 14, 2016
Open
@mdempsky
Copy link
Owner

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

@mdempsky
Copy link
Owner

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

@mdempsky
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mdempsky mdempsky

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mdempsky @mewmew