Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go/printer: /*-style comments placed after commas in argument lists #3062

Closed
griesemer opened this issue Feb 17, 2012 · 6 comments
Closed

go/printer: /*-style comments placed after commas in argument lists #3062

griesemer opened this issue Feb 17, 2012 · 6 comments

Comments

@griesemer
Copy link
Contributor

What steps will reproduce the problem?
- Apply gofmt to code containing lists with comments: (a, b /* comment */, c)

What is the expected output? What do you see instead?
- The comment should be placed correctly, but it's moved after the comma: (a, b, /*
comment */ c)

See: src/pkg/go/scanner/example_test.go for an example that was manually corrected.
Should update that code if this is fixed.
@rsc
Copy link
Contributor

rsc commented Feb 17, 2012

Comment 1:

Probably not important for Go 1.

Labels changed: added priority-later, removed priority-triage.

@griesemer
Copy link
Contributor Author

Comment 2:

This issue was closed by revision 8b7cdb7.

Status changed to Fixed.

@dgryski
Copy link
Contributor

dgryski commented Feb 22, 2012

Comment 3:

I just built this revision, and it looks like the problem has just changed which
comments are moved?  I think the proper fix should be: don't move comments to the other
side of commas.
My examples:
        var _ = []int{/* 1 */ x, /* 2 */ y}   /* comments in front of list elements */
        var _ = []int{x /* 1 */, y /* 2 */ }  /* comments after list elements */
Running through gofmt produces:
        var _ = []int{ /* 1 */ x /* 2 */, y}  /* comments in front of list elements */
        var _ = []int{x /* 1 */, y /* 2 */}   /* comments after list elements */
Previously, the second line was broken as the /* 1 */ comment would be moved to the
right of the comma next to the y, so the 'y' had two comments associated with it.
With the new code, we've instead broken the first case where the comments precede the
list elements.  The /* 2 */ on the first line has now moved to the left of the comma so
now the 'x' has two comments associated with it.

@rsc
Copy link
Contributor

rsc commented Feb 22, 2012

Comment 4:

/* comments in front of list elements */
don't do that anymore.

@griesemer
Copy link
Contributor Author

Comment 5:

There is no position information for commas recorded in the AST. As a result, their
position has to be estimated. It appears to be a more common style to put a comment
after a list entry than before; the new heuristic takes that into account. The existing
code base supports this assumptions and the fix improved comment placement for several
packages with such comments.
One could selectively store comma position information where needed, but that effort
(code and space consumption) seems not justified given that the new heuristic appears to
work reasonably well (and the case of comments in comma-separated lists is rare, anyway).

@MichaelTJones
Copy link
Contributor

Comment 6:

Why not do the following: When there is at least one comment in a comma separated list,
and it is not where you expect it, insert a known phantom comment in each uncommented
position. On output, drop the phantom comments. (string address== phantom string)

@griesemer griesemer self-assigned this Mar 24, 2014
@golang golang locked and limited conversation to collaborators Jun 24, 2016
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Not a Go 1 issue, but appeared to be fairly easy to fix.

- Note that a few existing test cases look slightly worse but
  those cases were not representative for real code. All real
  code looks better now.

- Manual move of the comment in go/scanner/example_test.go
  before applying gofmt.

- gofmt -w $GOROOT/src $GOROOT/misc

Fixes golang#3062.

R=rsc
CC=golang-dev
https://golang.org/cl/5674093
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants