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

cmd/cover: mishandles go: comments #10378

Closed
dvyukov opened this issue Apr 8, 2015 · 5 comments
Closed

cmd/cover: mishandles go: comments #10378

dvyukov opened this issue Apr 8, 2015 · 5 comments
Assignees
Milestone

Comments

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Apr 8, 2015

Running with current tip.
syscall/syscall_linux_amd64.go mistransformed from:

package syscall
const _SYS_dup = SYS_DUP2
func Getpagesize() int { return 4096 }
//go:noescape
func gettimeofday(tv *Timeval) (err Errno)

to:

package syscall
const _SYS_dup = SYS_DUP2
func Getpagesize() int  { GoCover_140.Count[
//go:noescape
0] = 1; return 4096 }
func gettimeofday(tv *Timeval) (err Errno)

Reproduce by running (on linux/amd64):

$ go test -c -cover syscall
# syscall
/tmp/go-build072163369/syscall/_test/_obj_test/syscall_linux_amd64.go:12: can only use //go:noescape with external func implementations
@dvyukov dvyukov added the repo-main label Apr 8, 2015
@dvyukov dvyukov added this to the Go1.5 milestone Apr 8, 2015
@robpike
Copy link
Contributor

@robpike robpike commented Apr 8, 2015

Infeasible. We need a way to manage comments in the AST. The current API of go/ast does not provide the hooks.

@robpike robpike closed this Apr 8, 2015
@dvyukov dvyukov modified the milestones: Unplanned, Go1.5 Apr 8, 2015
@dvyukov
Copy link
Member Author

@dvyukov dvyukov commented Apr 8, 2015

Let's keep it open with milestone Unplanned. This renders coverage unusable for important use cases.
@griesemer

@dvyukov dvyukov reopened this Apr 8, 2015
@dvyukov
Copy link
Member Author

@dvyukov dvyukov commented Apr 8, 2015

I have to patch my syscall package to use coverage.
Probably we need to lang the change to syscall package (it merely reorders lines) in the context of this issue.

@dvyukov
Copy link
Member Author

@dvyukov dvyukov commented Apr 9, 2015

@griesemer Robert, isn't it go/printer bug? The comments are attached to function declaration, why it inserts comments in the middle of other statements?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Apr 9, 2015

@dvyukov Some comments are attached, some are not. In general, the AST support for comments is not ideal. Comments inside functions are not attached at all. There's complex code in the printer that merges comments and code by comparing position information.

I haven't looked at the specifics of this problem. A generally correct solution (for all comments) is very difficult with the current AST. Maintaining function (and general declaration comments) should be doable. There's also the ast.CommentMap which may work for this problem.

The long-term solution is a new AST where all comments are attached to nodes.

@dvyukov dvyukov closed this in 53a8ee5 Apr 10, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.