code review 12699048: cmd/api: eliminate duplicate package import work

src/cmd/api/goapi.go

 // Copyright 2011 The Go Authors.  All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
 // +build api_tool
 
 // Binary api computes the exported API of a set of Go packages.
 package main
 
 import (
@@ skipping 21 matching lines @@
 
 // Flags
 var (
         checkFile  = flag.String("c", "", "optional comma-separated filename(s) to check API against")
         allowNew   = flag.Bool("allow_new", true, "allow API additions")
         exceptFile = flag.String("except", "", "optional filename of packages that are allowed to change without triggering a failure in the tool")
         nextFile   = flag.String("next", "", "optional filename of tentative upcoming API features for the next release. This file can be lazily maintained. It only affects the delta warnings from the -c file printed on success.")
         verbose    = flag.Bool("v", false, "verbose debugging")
         forceCtx   = flag.String("contexts", "", "optional comma-separated list of <goos>-<goarch>[-cgo] to override default contexts.")
 )
-
-var ourTags = map[string]bool{}

[bradfitz, 2013/08/09 21:41:29]

add a comment.

// ourTags is the set of tags we care about over all contexts ...

[gri, 2013/08/09 21:46:42]

I see this being assigned to, but never used. dregg?

[rsc, 2013/08/09 22:38:07]

Yes, gone.

 
 // contexts are the default contexts which are scanned, unless
 // overridden by the -contexts flag.
 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: "linux", GOARCH: "arm", CgoEnabled: true},
         {GOOS: "linux", GOARCH: "arm"},
@@ skipping 63 matching lines @@
                         fmt.Printf("Go version is %q, ignoring -next %s\n", runtime.Version(), *nextFile)
                         *nextFile = ""
                 }
         }
 
         if *forceCtx != "" {
                 setContexts()
         }
         for _, c := range contexts {
                 c.Compiler = build.Default.Compiler
-                ourTags[c.GOOS] = true
-                ourTags[c.GOARCH] = true
-        }
-        ourTags["cgo"] = true
+        }
 
         var pkgNames []string
         if flag.NArg() > 0 {
                 pkgNames = flag.Args()
         } else {
                 stds, err := exec.Command("go", "list", "std").Output()
                 if err != nil {
                         log.Fatal(err)
                 }
                 pkgNames = strings.Fields(string(stds))
@@ skipping 244 matching lines @@
 
 func contains(list []string, s string) bool {
         for _, t := range list {
                 if t == s {
                         return true
                 }
         }
         return false
 }
 
 var (

[gri, 2013/08/09 21:46:42]

please add comment about the key

// maps tag-based key -> package

?

[rsc, 2013/08/09 22:38:07]

Done.

-        pkgCache = map[string]*types.Package{}
-        pkgTags  = map[string][]string{}

[gri, 2013/08/09 21:46:42]

same here

[rsc, 2013/08/09 22:38:07]

Done.

+        pkgCache = map[string]*types.Package{} // map tagKey to package
+        pkgTags  = map[string][]string{}       // map import dir to list of relevant tags
 )
 
 // tagKey returns the tag-based key to use in the pkgCache.
 // It is a comma-separated string; the first part is dir, the rest tags.

[bradfitz, 2013/08/09 21:41:29]

we have structs that support == nowadays.  just make a "tagKey" type and lose
the English?

[rsc, 2013/08/09 22:38:07]

I'd still need the comma-separated key list since I have a variable number of
strings to use in the key. Left as is.

 // The satisfied tags are derived from context but only those that
 // matter (the ones listed in the tags argument) are used.
+// The tags list, which came from go/build's Package.AllTags,
+// is known to be sorted.
 func tagKey(dir string, context *build.Context, tags []string) string {
         ctags := map[string]bool{
                 context.GOOS:   true,
                 context.GOARCH: true,
         }
         if context.CgoEnabled {
                 ctags["cgo"] = true
         }
         for _, tag := range context.BuildTags {
                 ctags[tag] = true
         }
         // TODO: ReleaseTags (need to load default)
         key := dir

[bradfitz, 2013/08/09 21:41:29]

var keyTags []string
for _, tag := range tags {
   if ctags[tag] {
       keyTags = append(keyTags, tag)
   }
}
strings.Sort(keyTags)  // is this guaranteed otherwise?
return tagKey{dir, strings.Join(keyTags, ",")}

[rsc, 2013/08/09 22:38:07]

The AllTags list is sorted, and the tags list came from AllTags. Added a
comment.

         for _, tag := range tags {
                 if ctags[tag] {
                         key += "," + tag
                 }
         }
         return key
 }
 
 // Importing is a sentinel taking the place in Walker.imported
 // for a package that is in the process of being imported.
@@ skipping 413 matching lines @@
 
         if _, dup := w.features[f]; dup {
                 panic("duplicate feature inserted: " + f)
         }
         w.features[f] = true
 
         if *verbose {
                 log.Printf("feature: %s", f)
         }
 }