[golang-dev] code review 13441043: go.tools/oracle: change -pos flag syntax from "file pos... (issue 13441043)

Reviewers: crawshaw1,

Message:
Hello crawshaw@google.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.tools


Description:
go.tools/oracle: change -pos flag syntax from "file pos-pos" to
file:pos-pos.

Pro: no shell quotation needed.
Con: can't be parsed by (the perpertually useless) Scanf.

Please review this at https://codereview.appspot.com/13441043/

Affected files:
    M cmd/oracle/main.go
    M cmd/oracle/oracle.el
    M oracle/oracle.go
    M oracle/oracle_test.go


Index: cmd/oracle/main.go
===================================================================
--- a/cmd/oracle/main.go
+++ b/cmd/oracle/main.go
@@ -27,11 +27,9 @@
    "code.google.com/p/go.tools/oracle"
   )

-// TODO(adonovan): use a format that permits spaces in filenames, and
-// doesn't require shell quoting.
   var posFlag = flag.String("pos", "",
    "Filename and offset or extent of a syntax element about which to
query, "+
- "e.g. 'foo.go 123-456', 'bar.go 123'.")
+ "e.g. foo.go:123-456, bar.go:123.")

   var modeFlag = flag.String("mode", "",
    "Mode of query to perform: callers, callees, callstack, callgraph,
describe.")
Index: cmd/oracle/oracle.el
===================================================================
--- a/cmd/oracle/oracle.el
+++ b/cmd/oracle/oracle.el
@@ -75,11 +75,14 @@
     (if (string-equal "" go-oracle-scope)
         (go-oracle-set-scope))
     (let* ((filename (file-truename buffer-file-name))
- (pos (if (use-region-p)
- (format "%s-%s"
- (1- (go--position-bytes (region-beginning)))
- (1- (go--position-bytes (region-end))))
- (format "%s" (1- (position-bytes (point))))))
+ (posflag (if (use-region-p)
+ (format "-pos=%s:%s-%s"
+ filename
+ (1- (go--position-bytes (region-beginning)))
+ (1- (go--position-bytes (region-end))))
+ (format "-pos=%s:%s"
+ filename
+ (1- (position-bytes (point))))))
            ;; This would be simpler if we could just run 'go tool oracle'.
            (env-vars (go-root-and-paths))
            (goroot-env (concat "GOROOT=" (car env-vars)))
@@ -89,7 +92,7 @@
         (erase-buffer)
         (insert "Go Oracle\n")
         (let ((args (append (list go-oracle-command nil t nil
- (format "-pos=%s %s" filename pos)
+ posflag
                                   (format "-mode=%s" mode))
                             (split-string go-oracle-scope " " t))))
           ;; Log the command to *Messages*, for debugging.
Index: oracle/oracle.go
===================================================================
--- a/oracle/oracle.go
+++ b/oracle/oracle.go
@@ -21,6 +21,8 @@
    "io"
    "os"
    "path/filepath"
+ "strconv"
+ "strings"
    "time"

    "code.google.com/p/go.tools/go/types"
@@ -222,25 +224,43 @@
    return root
   }

-// parseQueryPos parses a string of the form "file pos" or file
-// start-end" where pos, start, end are decimal integers, and returns
-// the extent to which it refers.
+func parseDecimal(s string) int {
+ if s, err := strconv.ParseInt(s, 10, 32); err == nil {
+ return int(s)
+ }
+ return -1
+}
+
+// parseQueryPos parses a string of the form "file:pos" or
+// file:start-end" where pos, start, end are decimal integers, and
+// returns the extent to which it refers.
   //
   func parseQueryPos(fset *token.FileSet, queryPos string) (start, end
token.Pos, err error) {
    if queryPos == "" {
     err = fmt.Errorf("no source position specified (-pos flag)")
     return
    }
- var filename string
- var startOffset, endOffset int
- n, err := fmt.Sscanf(queryPos, "%s %d-%d", &filename, &startOffset,
&endOffset)
- if n != 3 {
- n, err = fmt.Sscanf(queryPos, "%s %d", &filename, &startOffset)
- if n != 2 {
- err = fmt.Errorf("invalid source position -pos=%q", queryPos)
- return
- }
+
+ colon := strings.LastIndex(queryPos, ":")
+ if colon < 0 {
+ err = fmt.Errorf("invalid source position -pos=%q", queryPos)
+ return
+ }
+ filename, offset := queryPos[:colon], queryPos[colon+1:]
+ startOffset := -1
+ endOffset := -1
+ if hyphen := strings.Index(offset, "-"); hyphen < 0 {
+ // e.g. "foo.go:123"
+ startOffset = parseDecimal(offset)
     endOffset = startOffset
+ } else {
+ // e.g. "foo.go:123-456"
+ startOffset = parseDecimal(offset[:hyphen])
+ endOffset = parseDecimal(offset[hyphen+1:])
+ }
+ if startOffset < 0 || endOffset < 0 {
+ err = fmt.Errorf("invalid -pos offset %q", offset)
+ return
    }

    var file *token.File
Index: oracle/oracle_test.go
===================================================================
--- a/oracle/oracle_test.go
+++ b/oracle/oracle_test.go
@@ -167,7 +167,7 @@
    buildContext.GOPATH = "testdata"
    err := oracle.Main([]string{q.filename},
     q.verb,
- fmt.Sprintf("%s %d-%d", q.filename, q.start, q.end),
+ fmt.Sprintf("%s:%d-%d", q.filename, q.start, q.end),
     /*PTA-log=*/ nil, capture, &buildContext)

    for _, line := range strings.Split(capture.String(), "\n") {