FAQ
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#newcode895
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#newcode1001
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#newcode1017
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)

https://codereview.appspot.com/6970051/

Search Discussions

  • Adg at Dec 20, 2012 at 5:49 am
    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#newcode894
    src/cmd/godoc/godoc.go:894: // with a matching name
    period.

    https://codereview.appspot.com/6970051/diff/6003/src/cmd/godoc/godoc.go#newcode908
    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#newcode934
    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#newcode935
    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#newcode942
    src/cmd/godoc/godoc.go:942: } else {
    no else needed after return

    https://codereview.appspot.com/6970051/
  • Kamil Kisiel at Dec 20, 2012 at 6:03 am
    Hello golang-dev@googlegroups.com, adg@golang.org (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/6970051/
  • Andrew Gerrand at Dec 20, 2012 at 5:51 am
    By the way, you'll need to fill out the CLA form before this change can be
    accepted. See: golang.org/doc/contribute.html#Copyright

    On 20 December 2012 16:43, wrote:

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

    Please take another look.


    https://codereview.appspot.**com/6970051/<https://codereview.appspot.com/6970051/>
  • Kamil Kisiel at Dec 20, 2012 at 6:03 am
    Reviewers: golang-dev_googlegroups.com,

    Message:
    Hello golang-dev@googlegroups.com,

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


    Description:
    cmd/godoc: ignore misnamed examples and print a warning

    Fixes issue 4211.

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

    Affected files:
    M src/cmd/godoc/godoc.go


    Index: src/cmd/godoc/godoc.go
    ===================================================================
    --- a/src/cmd/godoc/godoc.go
    +++ b/src/cmd/godoc/godoc.go
    @@ -317,18 +317,21 @@

    var exampleOutputRx = regexp.MustCompile(`(?i)//[[:space:]]*output:`)

    +// stripExampleSuffix strips lowercase braz in Foo_braz or Foo_Bar_braz
    from name
    +// while keeping uppercase Braz in Foo_Braz.
    +func stripExampleSuffix(name string) string {
    + if i := strings.LastIndex(name, "_"); i != -1 {
    + if i < len(name)-1 && !startsWithUppercase(name[i+1:]) {
    + name = name[:i]
    + }
    + }
    + return name
    +}
    +
    func example_htmlFunc(funcName string, examples []*doc.Example, fset
    *token.FileSet) string {
    var buf bytes.Buffer
    for _, eg := range examples {
    - name := eg.Name
    -
    - // Strip lowercase braz in Foo_braz or Foo_Bar_braz from name
    - // while keeping uppercase Braz in Foo_Braz.
    - if i := strings.LastIndex(name, "_"); i != -1 {
    - if i < len(name)-1 && !startsWithUppercase(name[i+1:]) {
    - name = name[:i]
    - }
    - }
    + name := stripExampleSuffix(eg.Name)

    if name != funcName {
    continue
    @@ -887,6 +890,39 @@
    }
    }

    +// declInPackages returns true if any of the packages in p have a
    declaration
    +// with the given name
    +func declInPackages(p map[string]*ast.Package, name string) bool {
    + for _, pkg := range p {
    + for _, file := range pkg.Files {
    + for _, decl := range file.Decls {
    + switch d := decl.(type) {
    + case *ast.FuncDecl:
    + if d.Name.Name == name {
    + return true
    + }
    + case *ast.GenDecl:
    + for _, spec := range d.Specs {
    + switch s := spec.(type) {
    + case *ast.TypeSpec:
    + if s.Name.Name == name {
    + return true
    + }
    + case *ast.ValueSpec:
    + for _, id := range s.Names {
    + if id.Name == name {
    + return true
    + }
    + }
    + }
    + }
    + }
    + }
    + }
    + }
    + return false
    +}
    +
    // getPageInfo returns the PageInfo for a package directory abspath. If the
    // parameter genAST is set, an AST containing only the package exports is
    // computed (PageInfo.PAst), otherwise package documentation (PageInfo.Doc)
    @@ -973,7 +1009,14 @@
    for _, f := range testpkg.Files {
    files = append(files, f)
    }
    - examples = append(examples, doc.Examples(files...)...)
    + for _, e := range doc.Examples(files...) {
    + name := stripExampleSuffix(e.Name)
    + if name == "" || declInPackages(pkgs, name) {
    + examples = append(examples, e)
    + } else {
    + log.Println("skipping example Example" + e.Name + ": refers to
    unknown function or type")
    + }
    + }
    }
    }
  • Adg at Dec 20, 2012 at 6:14 am
    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#newcode951
    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.

    https://codereview.appspot.com/6970051/
  • Kamil Kisiel at Dec 20, 2012 at 4:12 pm
    Hello golang-dev@googlegroups.com, adg@golang.org (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/6970051/
  • Kamil Kisiel at Dec 20, 2012 at 4:12 pm
    Hello golang-dev@googlegroups.com, adg@golang.org (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/6970051/
  • Kamil Kisiel at Dec 20, 2012 at 4:12 pm
    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#newcode951
    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&nbsp;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...

    https://codereview.appspot.com/6970051/
  • Adg at Dec 20, 2012 at 8:21 pm
    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#newcode895
    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#newcode896
    src/cmd/godoc/godoc.go:896: var names []string
    delete

    https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go#newcode915
    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#newcode923
    src/cmd/godoc/godoc.go:923: fmt.Println(names)
    delete

    https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go#newcode924
    src/cmd/godoc/godoc.go:924: return names
    return

    https://codereview.appspot.com/6970051/diff/7002/src/cmd/godoc/godoc.go#newcode927
    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#newcode960
    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#newcode961
    src/cmd/godoc/godoc.go:961: if name == "" || hasName {
    if name == "" || globals[name] {

    https://codereview.appspot.com/6970051/
  • Kamil Kisiel at Dec 21, 2012 at 8:13 am
    Hello golang-dev@googlegroups.com, adg@golang.org (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/6970051/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 20, '12 at 4:59a
activeDec 21, '12 at 8:13a
posts11
users2
websitegolang.org

2 users in discussion

Kamil Kisiel: 6 posts Adg: 5 posts

People

Translate

site design / logo © 2022 Grokbase