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

Jump to bottom

doc: go:noinline directives in godoc #34276

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

doc: go:noinline directives in godoc #34276

randall77 opened this issue Sep 12, 2019 · 7 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@randall77
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

bcmills commented Sep 13, 2019 •
edited

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
Copy link
Contributor

bcmills 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
Copy link
Contributor

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
Copy link
Contributor

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 The path to resolution is known, but the work has not been done. label Sep 16, 2019
@gopherbot
Copy link

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
@randall77
runtime: remove unneeded noinline directives
07ad840 
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
Copy link
Contributor Author

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

@golang golang locked and limited conversation to collaborators Oct 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@toothrot @rsc @agnivade @ianlancetaylor @bcmills @randall77 @gopherbot