Track Position rather than Offset #12

tamird · 2016-07-29T11:54:51Z

Fixes #3.

tamird · 2016-07-29T12:53:49Z

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

Fixes #3.

tamird · 2016-07-31T19:39:28Z

@mdempsky any other outstanding issues here?

mdempsky · 2016-08-02T17:52:41Z

unconvert.go

-			line := lineForOffset(buf, pos.Offset)
-			fmt.Printf("%s\n", line)
-			fmt.Printf("%s^\n", rub(line[:pos.Column-1]))
+			line := lineForPos(pos)


Seems unfortunate to need to re-read the source file for each warning message. What was wrong with the previous method of slurping it into memory?

well, it was being done even when the file wasn't needed because flagV wasn't provided. Made this better, though.

mdempsky · 2016-08-02T18:18:10Z

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 · 2016-08-02T18:46:38Z

@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 (8761692#diff-d45c7d690c87bd02c6ddd98b0dbd510fL120) which takes an offset and returns a Position (but does so incorrectly in the presence of cgo).

mdempsky · 2016-08-02T19:00:26Z

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

mdempsky · 2016-08-02T19:04:25Z

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 · 2016-08-02T19:09:13Z

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 · 2016-08-02T19:23:24Z

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 · 2016-08-02T19:55:33Z

So I'm getting at two things:

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.

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 · 2016-08-02T20:04:19Z

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:{}]

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 · 2016-08-02T20:05:01Z

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 · 2016-08-02T20:43:52Z

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 · 2016-08-02T20:48:27Z

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

mdempsky · 2016-08-02T20:57:30Z

Thanks!

Track Position rather than Offset

8761692

Fixes #3.

tamird mentioned this pull request Jul 29, 2016

fails on cockroachdb #11

Closed

mdempsky reviewed Aug 2, 2016
View reviewed changes

mdempsky merged commit 6cd281c into mdempsky:master Aug 2, 2016

mdempsky mentioned this pull request Aug 2, 2016

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

Closed

tamird deleted the use-position-not-offset branch August 2, 2016 21:09

tamird mentioned this pull request Aug 2, 2016

misc cleanup #13

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track Position rather than Offset #12

Track Position rather than Offset #12

tamird commented Jul 29, 2016

tamird commented Jul 29, 2016

tamird commented Jul 31, 2016

mdempsky Aug 2, 2016

tamird Aug 2, 2016

mdempsky commented Aug 2, 2016

tamird commented Aug 2, 2016

mdempsky commented Aug 2, 2016

mdempsky commented Aug 2, 2016

mdempsky commented Aug 2, 2016

tamird commented Aug 2, 2016

mdempsky commented Aug 2, 2016 •

edited

Loading

tamird commented Aug 2, 2016 •

edited

Loading

tamird commented Aug 2, 2016

mdempsky commented Aug 2, 2016 •

edited

Loading

tamird commented Aug 2, 2016

mdempsky commented Aug 2, 2016

Track Position rather than Offset #12

Track Position rather than Offset #12

Conversation

tamird commented Jul 29, 2016

tamird commented Jul 29, 2016

tamird commented Jul 31, 2016

mdempsky Aug 2, 2016

Choose a reason for hiding this comment

tamird Aug 2, 2016

Choose a reason for hiding this comment

mdempsky commented Aug 2, 2016

tamird commented Aug 2, 2016

mdempsky commented Aug 2, 2016

mdempsky commented Aug 2, 2016

mdempsky commented Aug 2, 2016

tamird commented Aug 2, 2016

mdempsky commented Aug 2, 2016 • edited Loading

tamird commented Aug 2, 2016 • edited Loading

tamird commented Aug 2, 2016

mdempsky commented Aug 2, 2016 • edited Loading

tamird commented Aug 2, 2016

mdempsky commented Aug 2, 2016

mdempsky commented Aug 2, 2016 •

edited

Loading

tamird commented Aug 2, 2016 •

edited

Loading

mdempsky commented Aug 2, 2016 •

edited

Loading