code review 6970051: cmd/godoc: ignore misnamed examples and print a warning

cmd/godoc: ignore misnamed examples and print a warning Fixes issue 4211.

[kisielk, 2012-12-20 04:27:35]

Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go

[adg, 2012-12-20 04:59:22]

https://codereview.appspot.com/6970051/diff/3001/src/cmd/godoc/godoc.go File src/cmd/godoc/godoc.go (right): https://codereview.appspot.com/6970051/diff/3001/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:895: func declInPackages(p map[string]*ast.Package, name string) bool { split this into two functions func hasGlobalName(p map[string]*ast.Package, name string) bool func hasName(decl ast.Decl, name string) bool so that the former is just the three for loops, and the latter is the switch statement. https://codereview.appspot.com/6970051/diff/3001/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:1001: filter = func(d os.FileInfo) bool { this is getting big, please move lines 1000 through 1019 to a function, so that it becomes: examples, err := parseExamples(fset, path) if err != nil { log.Println("parsing examples:", err) } https://codereview.appspot.com/6970051/diff/3001/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:1017: log.Println("skipping example Example" + e.Name + ": refers to unknown function or type") log.Printf("ignoring Example%s: refers to unknown declaration", e.Name)

[kisielk, 2012-12-20 05:43:04]

Hello golang-dev@googlegroups.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.

[adg, 2012-12-20 05:49:33]

https://codereview.appspot.com/6970051/diff/6003/src/cmd/godoc/godoc.go File src/cmd/godoc/godoc.go (right): https://codereview.appspot.com/6970051/diff/6003/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:894: // with a matching name period. https://codereview.appspot.com/6970051/diff/6003/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:908: // hasName returns true if decl has a matching name period. https://codereview.appspot.com/6970051/diff/6003/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:934: // parseExamples gets examples for packages in pkgs from *_test.go files in abspath s/gets/reads/ s/abspath/dir/ period. https://codereview.appspot.com/6970051/diff/6003/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:935: func parseExamples(fset *token.FileSet, pkgs map[string]*ast.Package, abspath string) ([]*doc.Example, error) { s/abspath/dir/ https://codereview.appspot.com/6970051/diff/6003/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:942: } else { no else needed after return

[kisielk, 2012-12-20 06:03:58]

Hello golang-dev@googlegroups.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.

[adg, 2012-12-20 06:14:03]

https://codereview.appspot.com/6970051/diff/2002/src/cmd/godoc/godoc.go File src/cmd/godoc/godoc.go (right): https://codereview.appspot.com/6970051/diff/2002/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:951: if name == "" || hasGlobalName(pkgs, name) { I just realized: this hasGlobalName function is pretty expensive, and we're doing it for each example in the package. It might be better to write a function: function globalNames(pkgs map[string]*ast.Package) map[string]bool that returns all names in those packages. Call that function once, outside of these loops. Here, you'd just check for the presence of the name in the map; cheap. Furthermore, I don't think this code works with methods. The globalNames function should, for each method declaration, return the name in the form Receiver_Method; the same form we use for the example functions.

[kisielk, 2012-12-20 07:35:41]

Hello golang-dev@googlegroups.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.

[kisielk, 2012-12-20 07:38:19]

https://codereview.appspot.com/6970051/diff/2002/src/cmd/godoc/godoc.go File src/cmd/godoc/godoc.go (right): https://codereview.appspot.com/6970051/diff/2002/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:951: if name == "" || hasGlobalName(pkgs, name) { On 2012/12/20 06:14:03, adg wrote: > I just realized: this hasGlobalName function is pretty expensive, and we're > doing it for each example in the package. > > It might be better to write a function: > > function globalNames(pkgs map[string]*ast.Package) map[string]bool > > that returns all names in those packages. Call that function once, outside of > these loops. Here, you'd just check for the presence of the name in the map; > cheap. > > Furthermore, I don't think this code works with methods. The globalNames > function should, for each method declaration, return the name in the form > Receiver_Method; the same form we use for the example functions. Right on both counts. I was just testing with the errors package which didn't have any methods. The updated version I just posted I tested with both errors and regexp and it seems to work correctly in both cases. I'm a little iffy about the type conversions used when dealing with the AST in the new changeset...

[adg, 2012-12-20 20:21:48]

Way better! https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go File src/cmd/godoc/godoc.go (right): https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:895: func declName(decl ast.Decl) []string { func declNames(decl ast.Decl) (names []string) https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:896: var names []string delete https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:915: names = []string{s.Name.Name} names = append(names, s.Name.Name) there can be more than one spec https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:923: fmt.Println(names) delete https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:924: return names return https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:927: // globalNames returns a map with the values of keys corresponding to global names in pks set to true. // globalNames finds all top-level declarations in pkgs and returns a map // with the identifier names as keys. https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:960: _, hasName := globals[name] delete can use the bool value instead, it defaults to false if key not present https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go#newco... src/cmd/godoc/godoc.go:961: if name == "" || hasName { if name == "" || globals[name] {

[kisielk, 2012-12-21 06:08:24]

Hello golang-dev@googlegroups.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.

[adg, 2013-01-02 04:11:53]

LGTM

[adg, 2013-01-02 05:00:58]

*** Submitted as https://code.google.com/p/go/source/detail?r=0dfd8fcdbca5 *** cmd/godoc: ignore misnamed examples and print a warning Fixes issue 4211. R=golang-dev, adg CC=golang-dev https://codereview.appspot.com/6970051 Committer: Andrew Gerrand <adg@golang.org>