code review 5626045: cmd/api: compare multiple contexts

cmd/api: compare multiple contexts

[bradfitz, 2012-02-03 19:45:09]

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

I'd like you to review this change to
https://go.googlecode.com/hg

[bradfitz, 2012-02-03 19:49:39]

I forgot to mail this out a few days ago.  I'm also not totally happy with
it:

Currently when all contexts don't match, each context is printed for a
feature, even when it's pretty redundant:

pkg os (darwin-386), func NewFile(int, string) *File
pkg os (darwin-386-cgo), func NewFile(int, string) *File
pkg os (darwin-amd64), func NewFile(int, string) *File
pkg os (darwin-amd64-cgo), func NewFile(int, string) *File
pkg os (linux-386), func NewFile(int, string) *File
pkg os (linux-386-cgo), func NewFile(int, string) *File
pkg os (linux-amd64), func NewFile(int, string) *File
pkg os (linux-amd64-cgo), func NewFile(int, string) *File
pkg os (windows-386), func NewFile(syscall.Handle, string) *File
pkg os (windows-amd64), func NewFile(syscall.Handle, string) *File

It'd be nicer to dump it out smarter, with just unique prefixes that
differ.  Like maybe:

pkg os (darwin-*), func NewFile(int, string) *File
pkg os (linux-*), func NewFile(int, string) *File
pkg os (windows-*), func NewFile(syscall.Handle, string) *File

And then probably change "linux-386" to "linux-386-nocgo" instead, so it
has the same depth.

But I don't care enough, unless others do too.


On Fri, Feb 3, 2012 at 11:45 AM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
> cmd/api: compare multiple contexts
>
> Please review this at
http://codereview.appspot.com/**5626045/<http://codereview.appspot.com/5626045/>
>
> Affected files:
>  M src/cmd/api/goapi.go
>
>
> Index: src/cmd/api/goapi.go
> ==============================**==============================**=======
> --- a/src/cmd/api/goapi.go
> +++ b/src/cmd/api/goapi.go
> @@ -34,6 +34,27 @@
>        verbose   = flag.Bool("v", false, "Verbose debugging")
>  )
>
> +var contexts = []*build.Context{
> +       {GOOS: "linux", GOARCH: "386", CgoEnabled: true},
> +       {GOOS: "linux", GOARCH: "386"},
> +       {GOOS: "linux", GOARCH: "amd64", CgoEnabled: true},
> +       {GOOS: "linux", GOARCH: "amd64"},
> +       {GOOS: "darwin", GOARCH: "386", CgoEnabled: true},
> +       {GOOS: "darwin", GOARCH: "386"},
> +       {GOOS: "darwin", GOARCH: "amd64", CgoEnabled: true},
> +       {GOOS: "darwin", GOARCH: "amd64"},
> +       {GOOS: "windows", GOARCH: "amd64"},
> +       {GOOS: "windows", GOARCH: "386"},
> +}
> +
> +func contextName(c *build.Context) string {
> +       s := c.GOOS + "-" + c.GOARCH
> +       if c.CgoEnabled {
> +               return s + "-cgo"
> +       }
> +       return s
> +}
> +
>  func main() {
>        flag.Parse()
>
> @@ -48,28 +69,54 @@
>                pkgs = strings.Fields(string(stds))
>        }
>
> -       w := NewWalker()
>        tree, _, err := build.FindTree("os") // some known package
>        if err != nil {
>                log.Fatalf("failed to find tree: %v", err)
>        }
> -       w.tree = tree
>
> -       for _, pkg := range pkgs {
> -               w.wantedPkg[pkg] = true
> +       var featureCtx = make(map[string]map[string]**bool) // feature ->
> context name -> true
> +       for _, context := range contexts {
> +               w := NewWalker()
> +               w.context = context
> +               w.tree = tree
> +
> +               for _, pkg := range pkgs {
> +                       w.wantedPkg[pkg] = true
> +               }
> +
> +               for _, pkg := range pkgs {
> +                       if strings.HasPrefix(pkg, "cmd/") ||
> +                               strings.HasPrefix(pkg, "exp/") ||
> +                               strings.HasPrefix(pkg, "old/") {
> +                               continue
> +                       }
> +                       if !tree.HasSrc(pkg) {
> +                               log.Fatalf("no source in tree for package
> %q", pkg)
> +                       }
> +                       w.WalkPackage(pkg)
> +               }
> +               ctxName := contextName(context)
> +               for _, f := range w.Features() {
> +                       if _, ok := featureCtx[f]; !ok {
> +                               featureCtx[f] = make(map[string]bool)
> +                       }
> +                       featureCtx[f][ctxName] = true
> +               }
>        }
>
> -       for _, pkg := range pkgs {
> -               if strings.HasPrefix(pkg, "cmd/") ||
> -                       strings.HasPrefix(pkg, "exp/") ||
> -                       strings.HasPrefix(pkg, "old/") {
> +       var features []string
> +       for f, cmap := range featureCtx {
> +               if len(cmap) == len(contexts) {
> +                       features = append(features, f)
>                        continue
>                }
> -               if !tree.HasSrc(pkg) {
> -                       log.Fatalf("no source in tree for package %q", pkg)
> +               comma := strings.Index(f, ",")
> +               for cname := range cmap {
> +                       f2 := fmt.Sprintf("%s (%s)%s", f[:comma], cname,
> f[comma:])
> +                       features = append(features, f2)
>                }
> -               w.WalkPackage(pkg)
>        }
> +       sort.Strings(features)
>
>        bw := bufio.NewWriter(os.Stdout)
>        defer bw.Flush()
> @@ -81,7 +128,7 @@
>                }
>                v1 := strings.Split(string(bs), "\n")
>                sort.Strings(v1)
> -               v2 := w.Features()
> +               v2 := features
>                take := func(sl *[]string) string {
>                        s := (*sl)[0]
>                        *sl = (*sl)[1:]
> @@ -99,7 +146,7 @@
>                        }
>                }
>        } else {
> -               for _, f := range w.Features() {
> +               for _, f := range features {
>                        fmt.Fprintf(bw, "%s\n", f)
>                }
>        }
> @@ -112,6 +159,7 @@
>  }
>
>  type Walker struct {
> +       context         *build.Context
>        tree            *build.Tree
>        fset            *token.FileSet
>        scope           []string
> @@ -221,8 +269,17 @@
>        }()
>        dir := filepath.Join(w.tree.SrcDir(), filepath.FromSlash(name))
>
> -       info, err := build.ScanDir(dir)
> +       var info *build.DirInfo
> +       var err error
> +       if ctx := w.context; ctx != nil {
> +               info, err = ctx.ScanDir(dir)
> +       } else {
> +               info, err = build.ScanDir(dir)
> +       }
>        if err != nil {
> +               if strings.Contains(err.Error(), "no Go source files") {
> +                       return
> +               }
>                log.Fatalf("pkg %q, dir %q: ScanDir: %v", name, dir, err)
>        }
>
>
>
>

[rsc, 2012-02-03 19:50:54]

I don't think it's worth the effort to shorten.

[bradfitz, 2012-02-03 20:10:29]

review away, then.

On Fri, Feb 3, 2012 at 11:50 AM, Russ Cox <rsc@golang.org> wrote:

> I don't think it's worth the effort to shorten.
>

[rsc, 2012-02-03 20:33:49]

LGTM

http://codereview.appspot.com/5626045/diff/1002/src/cmd/api/goapi.go
File src/cmd/api/goapi.go (right):

http://codereview.appspot.com/5626045/diff/1002/src/cmd/api/goapi.go#newcode100
src/cmd/api/goapi.go:100: if _, ok := featureCtx[f]; !ok {
if featureCtx[f] == nil {

[bradfitz, 2012-02-08 02:13:18]

*** Submitted as http://code.google.com/p/go/source/detail?r=73a4a2506d41 ***

cmd/api: compare multiple contexts

R=golang-dev, rsc
CC=golang-dev
http://codereview.appspot.com/5626045