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

doc: go:noinline directives in godoc #34276

Closed
randall77 opened this issue Sep 12, 2019 · 7 comments

Comments

@randall77
Copy link
Contributor

commented Sep 12, 2019

In the docs for runtime.Callers https://golang.org/pkg/runtime/#Callers , the text ends with go:noinline. We probably shouldn't include that part (or any go: directive) in the godoc.

This occurs in some internal packages as well, like runtime/internal/atomic.

@randall77 randall77 added this to the Go1.14 milestone Sep 12, 2019
@agnivade

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

I think this should be handled in go/doc.Func itself. When doc.New is called, Func.Doc should not contain go: directives.

@griesemer

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

My impression from previous discussions (such as #32816 (comment)) is that //go: directives are intended to be placed before the affected declaration, separated from the doc comment by a blank line. Does that work for //go:noinline directives?

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

That does, at least, suggest that we should have a vet or lint warning for misplaced //go: directives. (CC @ianthehat @dominikh)

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

I don't think we should have godoc remove these comments, as that would be confusing when the function's doc is in fact intended to include such a comment. Similarly, I don't think a go vet warning would be appropriate, although a golint one would be fine.

Let's just fix the cases as @bcmills suggests, which is how it is supposed to be used anyhow.

@toothrot

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Adding NeedsFix, as positioning the //go: directives resolves the issue, and it seems straightforward.

runtime.KeepAlive is a good example:

go/src/runtime/mfinal.go

Lines 424 to 427 in 115e4c9

// Mark KeepAlive as noinline so that it is easily detectable as an intrinsic.
//go:noinline
// KeepAlive marks its argument as currently reachable.

runtime.KeepAlive docs: https://godoc.org/runtime#KeepAlive

@toothrot toothrot added the NeedsFix label Sep 16, 2019
@gopherbot

This comment has been minimized.

Copy link

commented Sep 16, 2019

Change https://golang.org/cl/195818 mentions this issue: runtime: remove unneeded noinline directives

gopherbot pushed a commit that referenced this issue Sep 17, 2019
Now that mid-stack inlining reports backtraces correctly, we no
longer need to protect against inlining in a few critical areas.

Update #19348
Update #28640
Update #34276

Change-Id: Ie68487e6482c3a9509ecf7ecbbd40fe43cee8381
Reviewed-on: https://go-review.googlesource.com/c/go/+/195818
Reviewed-by: David Chase <drchase@google.com>
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@randall77

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

Closing. runtime.Callers has been fixed, and it sounds like we should just move the comments instead of fixing godoc.

@randall77 randall77 closed this Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.