Track Position rather than Offset

[tamird]

Fixes #3.

[tamird]

This still doesn't quite do the right thing in the presence of cgo (mapping back to the original source location is somewhat broken).

[tamird]

@mdempsky any other outstanding issues here?

[mdempsky]

I think I'm still confused how the Position change fixes anything. Like I said, token.Position is a superset of Offset. Do you have a test case that shows your change improves behavior?

[tamird]

@mdempsky yes, if you run master against cockroachdb as described in #11, you will get a panic.

After this PR, there is no panic. The reason is that there is no longer a call to file.Pos (https://github.com/mdempsky/unconvert/pull/12/commits/8761692960917b3cc37faeb1052ba5c1c1e77945#diff-d45c7d690c87bd02c6ddd98b0dbd510fL120) which takes an offset and returns a Position (but does so incorrectly in the presence of cgo).

[mdempsky]

Ohh, okay. I understand the issue now, thanks.

[mdempsky]

Hm, does -apply work for your test case? I'd expect it's going to run successfully (unlike the file.Pos panic), but the edits map lookup in (*editor).rewrite is never going to succeed because of pos.Offset.

[mdempsky]

Arg, this is even subtler actually. We can only rely on line numbers being correct for cgo-processed files. The column numbers may be wrong if cgo rewrite any expressions earlier in the same line.

[tamird]

I'm not sure what you're getting at, @mdempsky. Is there any reason not to merge this PR? we're currently using my fork in cockroachdb, and I'd like to resume using yours.

[mdempsky]

So I'm getting at two things:

1. I'm asking if using unconvert -apply works with cockroachdb in files that use cgo (i.e., that have import "C"). My expectation is no, but I could be pleasantly surprised.

2. If you have a line like:

    fmt.Println(C.int(0), C.int(0), C.int(0), int(int(0)), "hi")

I expect the column number for the redundant int conversion will be wrong.

My hesitation is that it seems to not fully solve the cgo case, and I'm looking to see if there's a better solution.

[tamird]

1. unconvert at master produces:

   $ unconvert -apply ./...
   2016/08/02 15:57:52 /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go: missing edits {9667 10030 14548 14653 16575 22455 23094 23696 24460 25111 26783 28052 39727 40261 44797 45433 46337 49275 49970 50855 51243 51675 52087 52435 53331 53703 54078 54452 56445 57488 62921 64003 64668 65092 65575 65661 66268 66354 67324}

   unconvert at this PR produces:

   unconvert -apply ./...
   2016/08/02 15:58:43 /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go: missing edits map[/Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1121:88:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1361:80:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1374:78:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1388:80:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1087:67:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:884:60:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1012:86:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1076:70:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1147:75:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1157:75:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:567:85:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1413:81:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1350:78:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:987:101:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1152:75:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1187:86:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1365:89:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1388:166:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:359:174:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:359:69:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:595:86:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:651:59:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1108:76:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1330:109:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1374:164:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:255:66:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:404:60:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:553:80:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:998:60:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1142:75:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:245:72:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:869:70:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:625:86:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:583:74:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1102:83:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1115:82:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1124:83:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:1202:80:{} /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/storage/engine/rocksdb.go:543:82:{}]

2. given the file foo/main.go:

   package main
   
   import "fmt"
   import "C"
   
   func main() {
      fmt.Println(C.int(0), C.int(0), C.int(0), int(int(0)))
   }

   unconvert at master produces:

   $ unconvert ./foo/
   panic: illegal file offset
   
   goroutine 1 [running]:
   panic(0x250080, 0xc8221f82d0)
      /Users/tamird/src/go1.6/src/runtime/panic.go:481 +0x3e6
   go/token.(*File).Pos(0xc8216d01e0, 0x213, 0x1)
      /Users/tamird/src/go1.6/src/go/token/position.go:237 +0x78
   main.print(0xc820013db0, 0x45, 0xc8216cbb10)
      /Users/tamird/src/go/src/github.com/mdempsky/unconvert/unconvert.go:120 +0x3ef
   main.main()
      /Users/tamird/src/go/src/github.com/mdempsky/unconvert/unconvert.go:220 +0x66b

   unconvert at this PR produces:

   unconvert ./foo/
   /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/foo/main.go:7:62: unnecessary conversion

[tamird]

I'm glad you're looking for a better solution. In the meantime, this fixes a panic, and slightly reduces the burden on me to keep rolling the boulder up the hill. What's the downside to accepting this patch?

[mdempsky]

Thanks for testing.

I just haven't finished reviewing the code yet, so I don't know yet what the downsides are, if any.

I'm okay with the first commit now.

The second cleanup commit makes a lot of changes that I don't understand and that don't seem related to each other, and so I'm currently still trying to understand them. I'd prefer if you sent those as a separate PR and preferably as multiple commits or at least a commit message explaining the individual changes.

[tamird]

Removed the second commit, will send as a separate PR after this is merged.

[mdempsky]

Thanks!