go/doc: fix identifier blank import handling for examples

go/doc: fix identifier blank import handling for examples
Also fixed one docs typo.

Fixes issue 4300.

[r, 2012-10-31 17:55:07]

LGTM

[minux1, 2012-10-31 19:52:44]

Hi rob,
    Do you think we need to restore comment group associated with the blank
identifier
import? For example, that image test has this comment before the import,
which i think
is very important.

// Package image/jpeg is not used explicitly in the code below,
// but is imported for its initialization side-effect, which allows
 // image.Decode to understand JPEG formatted images. Uncomment these
// two lines to also understand GIF and PNG images:
 // _ "image/gif"
// _ "image/png"

I have a partially working patch set (4), however I don't know why the
comment is
formatted wrong, like this:
// Package image/jpeg is not used explicitly in the code below,
// but is imported for its initialization side-effect, which allows
// image.Decode to understand JPEG formatted images. Uncomment these
// two lines to also understand GIF and PNG images:
// _ "image/gif"
// _ "image/png"
_ "image/jpeg"

[r, 2012-10-31 20:05:58]

It would be nice. Let's see if gri can help.

-rob

[gri, 2012-11-02 23:48:23]

NOT LGTM

There are more issues with this. Will have to look at this next week. See also
update on issue 4300.

https://codereview.appspot.com/6813061/diff/2002/src/pkg/go/doc/example.go
File src/pkg/go/doc/example.go (right):

https://codereview.appspot.com/6813061/diff/2002/src/pkg/go/doc/example.go#ne...
src/pkg/go/doc/example.go:184: if c.Pos() >= p.Doc.Pos() && c.Pos() <
p.Doc.End() {
I am getting a crash here; possibly because p.Doc is nil? See also update on
issue 4300.

[minux1, 2012-11-03 06:22:31]

On 2012/11/02 23:48:23, gri wrote:
https://codereview.appspot.com/6813061/diff/2002/src/pkg/go/doc/example.go#ne...
> src/pkg/go/doc/example.go:184: if c.Pos() >= p.Doc.Pos() && c.Pos() <
> p.Doc.End() {
> I am getting a crash here; possibly because p.Doc is nil? See also update on
> issue 4300.
Thank you for pointing that out, fixed.

Still didn't know why the formatting is wrong.

[minux1, 2012-11-09 10:54:09]

gentle ping.

[gri, 2012-11-10 00:21:09]

yes, my apologies. it's not the highest priority at the moment but
I'll get to it.
- gri

On Fri, Nov 9, 2012 at 2:54 AM,  <minux.ma@gmail.com> wrote:
> gentle ping.
>
> https://codereview.appspot.com/6813061/

[adg, 2012-11-13 11:50:47]

The general approach looks OK to me, but:

https://codereview.appspot.com/6813061/diff/9002/src/pkg/go/doc/example.go
File src/pkg/go/doc/example.go (right):

https://codereview.appspot.com/6813061/diff/9002/src/pkg/go/doc/example.go#ne...
src/pkg/go/doc/example.go:183: for _, p := range blankImports {
I think this code is getting complex enough that we should add some tests.

Do you think you could add some in this CL? If not, I can take a crack at it
later.

[minux1, 2012-11-15 11:04:09]

https://codereview.appspot.com/6813061/diff/9002/src/pkg/go/doc/example.go
File src/pkg/go/doc/example.go (right):

https://codereview.appspot.com/6813061/diff/9002/src/pkg/go/doc/example.go#ne...
src/pkg/go/doc/example.go:183: for _, p := range blankImports {
On 2012/11/13 11:50:48, adg wrote:
> Do you think you could add some in this CL? If not, I can take a crack at it
> later.
Will do that after I figure out why the formatting
is wrong. do you have any hints about that?

[gri, 2012-11-16 19:52:23]

Apologies for the delay, I've been busy with other stuff.

Some more comments on this CL. These suggestions don't change the alignment
issue we have - I am not sure yet why they exits (possibly a go/printer bug -
will investigate as next step).

The suggestions below apply to the existing code and should be made
nevertheless.

That said, the existing code could be much simpler and work better if it would
be using ast.CommentMaps. CommentMaps are here to make comment tracking and
filtering easy, and much of the code below dealing with comments could go away.
It would also work better: For instance, as is, comments associated with
non-blank imports are dropped. With comment maps, they wouldn't.

However, CommentMaps need the corresponding ast.FileSet. This means that the
doc.Examples function would need an additional *token.FileSet parameter. Since
this is an exported function this would be a non-backward compatible API change.

I think we should perhaps consider this (I think this function is only used by
godoc), but first we should get the existing code right. I will now look into
the alignment issue.

- gri

https://codereview.appspot.com/6813061/diff/9002/src/pkg/go/doc/example.go
File src/pkg/go/doc/example.go (right):

https://codereview.appspot.com/6813061/diff/9002/src/pkg/go/doc/example.go#ne...
src/pkg/go/doc/example.go:149: imports := make(map[string]string)  // [name]path
Please call the first map say "namedImports" so it's clear that it does not
include the blankImports. Also, if they are ast.Specs, then we don't need a loop
later to append them.

namedImports := make(map[string]string) // [name]path
var blankImports []ast.Spec // _ imports

https://codereview.appspot.com/6813061/diff/9002/src/pkg/go/doc/example.go#ne...
src/pkg/go/doc/example.go:168: if n == "_" {
This should be "else if", but better:

switch {
case unresolved[n]:
    namedImports[n] = p
    delete(unresolved, n)
case n == "_":
    blankImports = append(blankImports, s)
}

https://codereview.appspot.com/6813061/diff/9002/src/pkg/go/doc/example.go#ne...
src/pkg/go/doc/example.go:179: // Filter out comments that are outside the
function body.
This is too complicated. The comments associated with blank imports are known,
they are the corresponding .Doc. This can be much simplified to:

        // Include documentation belonging to blank imports.
	var comments []*ast.CommentGroup
	for _, s := range blankImports {
		if c := s.(*ast.ImportSpec).Doc; c != nil {
			comments = append(comments, c)
		}
	}

	// Include comments that are inside the function body.
	for _, c := range file.Comments {
		if body.Pos() <= c.Pos() && c.End() <= body.End() {
			comments = append(comments, c)
		}
	}

(Also, no need for 'continue' in the 2nd loop.)

https://codereview.appspot.com/6813061/diff/9002/src/pkg/go/doc/example.go#ne...
src/pkg/go/doc/example.go:206: for n, p := range imports {
s/imports/namedImports/

https://codereview.appspot.com/6813061/diff/9002/src/pkg/go/doc/example.go#ne...
src/pkg/go/doc/example.go:213: for _, p := range blankImports {
this becomes:

importDecl.Specs = append(importDecl.Specs, blankImports...)

[minux1, 2012-11-17 17:54:27]

On 2012/11/16 19:52:23, gri wrote:
> Some more comments on this CL. These suggestions don't change the alignment
> issue we have - I am not sure yet why they exits (possibly a go/printer bug -
> will investigate as next step).
Thank you for the comprehensive review.
I think CL 6782082 is better than this CL, so I will abandon this CL.

[minux1, 2012-11-17 17:55:07]

*** Abandoned ***