Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1117)

Issue 6209080: code review 6209080: go/parser: fix comment grouping (day 1 bug) (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 11 months ago by gri
Modified:
11 years, 11 months ago
Reviewers:
CC:
rsc, golang-dev
Visibility:
Public.

Description

go/parser: fix comment grouping (day 1 bug) Comment groups must end at the end of a line (or the next non-comment token) if the group started on a line with non-comment tokens. This is important for correct computation of "lead" and "line" comments (Doc and Comment fields in AST nodes). Without this fix, the "line" comment for F1 in the following example: type T struct { F1 int // comment1 // comment2 F2 int } is "// comment1// comment2" rather than just "// comment1". This bug was present from Day 1 but only visible when looking at export-filtered ASTs where only comments associated with AST nodes are printed, and only in rare cases (e.g, in the case above, if F2 where not exported, godoc would show "// comment2" anyway because it was considered part of the "line" comment for F1). The bug fix is very small (parser.go). The bulk of the changes are additional test cases (parser_test.go). The fix exposed a caching bug in go/printer via one of the existing tests, hence the changes to printer.go. As an aside, the fix removes the the need for empty lines before an "// Output" comment for some special cases of code examples (e.g.: src/pkg/strings/example_test.go, Count example). No impact on gofmt formatting of src, misc. Fixes issue 3139.

Patch Set 1 #

Patch Set 2 : diff -r 820ffde8c396 https://code.google.com/p/go #

Patch Set 3 : diff -r 820ffde8c396 https://code.google.com/p/go #

Patch Set 4 : diff -r 820ffde8c396 https://code.google.com/p/go #

Patch Set 5 : diff -r 820ffde8c396 https://code.google.com/p/go #

Patch Set 6 : diff -r 820ffde8c396 https://code.google.com/p/go #

Patch Set 7 : diff -r e92c07f19c3a https://code.google.com/p/go #

Patch Set 8 : diff -r e92c07f19c3a https://code.google.com/p/go #

Patch Set 9 : diff -r e92c07f19c3a https://code.google.com/p/go #

Patch Set 10 : diff -r b13f9b2f6794 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+147 lines, -15 lines) Patch
M src/pkg/go/parser/parser.go View 1 3 chunks +6 lines, -6 lines 0 comments Download
M src/pkg/go/parser/parser_test.go View 1 2 3 7 chunks +129 lines, -5 lines 0 comments Download
M src/pkg/go/printer/nodes.go View 1 2 3 4 5 6 2 chunks +12 lines, -3 lines 0 comments Download
M src/pkg/strings/example_test.go View 1 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 3
gri
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
11 years, 11 months ago (2012-05-19 00:34:06 UTC) #1
rsc
LGTM Apologies for the delay reviewing this. Rietveld says things about chunk errors instead of ...
11 years, 11 months ago (2012-05-22 03:09:30 UTC) #2
gri
11 years, 11 months ago (2012-05-22 17:04:38 UTC) #3
*** Submitted as http://code.google.com/p/go/source/detail?r=afbf8db1baf4 ***

go/parser: fix comment grouping (day 1 bug)

Comment groups must end at the end of a line (or the
next non-comment token) if the group started on a line
with non-comment tokens.

This is important for correct computation of "lead"
and "line" comments (Doc and Comment fields in AST nodes).

Without this fix, the "line" comment for F1 in the
following example:

type T struct {
     F1 int // comment1
     // comment2
     F2 int
}

is "// comment1// comment2" rather than just "// comment1".

This bug was present from Day 1 but only visible when
looking at export-filtered ASTs where only comments
associated with AST nodes are printed, and only in rare
cases (e.g, in the case above, if F2 where not exported,
godoc would show "// comment2" anyway because it was
considered part of the "line" comment for F1).

The bug fix is very small (parser.go). The bulk of the
changes are additional test cases (parser_test.go).

The fix exposed a caching bug in go/printer via one of the
existing tests, hence the changes to printer.go.

As an aside, the fix removes the the need for empty lines
before an "// Output" comment for some special cases of
code examples (e.g.: src/pkg/strings/example_test.go, Count
example).

No impact on gofmt formatting of src, misc.

Fixes issue 3139.

R=rsc
CC=golang-dev
http://codereview.appspot.com/6209080
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b