[go] runtime: clarify Frames.Next documentation
Matthew Dempsky (Gerrit)
Attention is currently required from: Ian Lance Taylor.
Matthew Dempsky would like Ian Lance Taylor to review this change.
runtime: clarify Frames.Next documentation
I wrote code that relied on this API, but I misunderstood the original
description of the "more" result. As a consequence, my code always
stopped one frame early.
This CL expands the documentation to be more explicit and specifically
call out my confusion (i.e., that the "more" result indicates whether
the *next* Next call will return a valid Frame, and not whether this
call did).
Change-Id: If135f8f8c05425073d45377c4179e4f79e6bd6ca
---
M src/runtime/symtab.go
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/runtime/symtab.go b/src/runtime/symtab.go
index 6b535dfc..e9f5bb2 100644
--- a/src/runtime/symtab.go
+++ b/src/runtime/symtab.go
@@ -68,8 +68,14 @@
return f
}
-// Next returns frame information for the next caller.
-// If more is false, there are no more callers (the Frame value is valid).
+// Next returns a Frame representing the next call frame in the slice
+// of PC values. If it has already returned all call frames, Next
+// returns a zero Frame instead.
+//
+// Next also reports whether there are still more call frames to
+// return after this one. That is, it reports whether another call to
+// Next would return a valid Frame, and not necessarily whether this
+// call returned a valid Frame.
func (ci *Frames) Next() (frame Frame, more bool) {
for len(ci.frames) < 2 {
// Find the next frame.
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Matthew Dempsky.
2 comments:
File src/runtime/symtab.go:
Patch Set #1, Line 73:
// returns a zero Frame instead.s/instead//
Patch Set #1, Line 76:
// return after this one. That is, it reports whether another call toI don't see how the second sentence adds anything to the first sentence. I guess this is the point of confusion, but I think it's best to try to state the behavior clearly rather than to follow up with a "That is". To me the first sentence you wrote here already states matters clearly and the "That is" is unnecessary.
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Dempsky (Gerrit)
Attention is currently required from: Matthew Dempsky.
Matthew Dempsky uploaded patch set #2 to this change.
runtime: clarify Frames.Next documentation
I wrote code that relied on this API, but I misunderstood the original
description of the "more" result. As a consequence, my code always
stopped one frame early.
This CL expands the documentation to be more explicit and specifically
call out my confusion (i.e., that the "more" result indicates whether
the *next* Next call will return a valid Frame, and not whether this
call did).
Change-Id: If135f8f8c05425073d45377c4179e4f79e6bd6ca
---
M src/runtime/symtab.go
1 file changed, 8 insertions(+), 2 deletions(-)
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Dempsky (Gerrit)
Attention is currently required from: Ian Lance Taylor.
Patch set 2:Trust +1
2 comments:
File src/runtime/symtab.go:
Patch Set #1, Line 73:
// returns a zero Frame instead.s/instead//
Done
Patch Set #1, Line 76:
// return after this one. That is, it reports whether another call to
I don't see how the second sentence adds anything to the first sentence. […]
Typical Go APIs follow the T,error or T,ok idiom, where error/ok indicate whether the T value is valid and should be processed. If error is non-nil or ok is false, you don't process T.
io.Reader.Read is a notable case that diverges from this: error may be non-nil, but you still need to process the n result first. Accordingly, even though it does already clearly state the behavior ("When Read encounters an error or end-of-file condition after successfully reading n > 0 bytes, it returns the number of bytes read."), it still spends an extra paragraph warning users about this ("Callers should always process the n > 0 bytes returned before considering the error err. ...").
The T,more semantics here are similarly atypical, and I think that merits emphasis. Namely, that the "more" result represents the state of the world *after* the current frame has been returned and whether future calls will succeed; it does *not* represent whether the current call succeeded, like T,error and T,ok forms do.
I can see an argument that naming the result "more" instead of "ok" should have hinted at this. But my experience is that replacing "ok" with a more descriptive word is still a more common practice than T,more is. I did not think anything of it when reading how to use this API.
If godoc supported markup, I'd simply put "after" in bold or italics to warn the reader that prepositional phrase is critical. But instead, I followed io.Reader's example and exercised godoc's existing "multiple sentences" feature.
I considered instead writing something like "Callers should always process the returned Frame before considering more." But that's still misleading: Next may have returned a zero Frame that should not be processed. So then even more words would be necessary to further caution about those cases.
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Matthew Dempsky.
1 comment:
File src/runtime/symtab.go:
Patch Set #1, Line 76:
// return after this one. That is, it reports whether another call toTypical Go APIs follow the T,error or T,ok idiom, where error/ok indicate whether the T value is val […]
I understand the confusion. But what I think we want to avoid in the docs is saying one thing and then adjusting it via words like "That is". We want to just say it.
How about something along the lines of
"The more result reports whether the next call to Next will return a valid Frame. If there are no frames available the first call to Next will return a zero Frame and a more result of false. Otherwise callers can process the Frame and then use the more result to decide whether to call Next again."
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Dempsky (Gerrit)
Attention is currently required from: Matthew Dempsky.
Matthew Dempsky uploaded patch set #3 to this change.
runtime: clarify Frames.Next documentation
I wrote code that relied on this API, but I misunderstood the original
description of the "more" result. As a consequence, my code always
stopped one frame early.
This CL expands the documentation to be more explicit and specifically
call out my confusion (i.e., that the "more" result indicates whether
the *next* Next call will return a valid Frame, and not whether this
call did).
Change-Id: If135f8f8c05425073d45377c4179e4f79e6bd6ca
---
M src/runtime/example_test.go
M src/runtime/symtab.go
2 files changed, 21 insertions(+), 6 deletions(-)
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Dempsky (Gerrit)
Attention is currently required from: Ian Lance Taylor.
Patch set 2:Trust +1
1 comment:
File src/runtime/symtab.go:
Patch Set #1, Line 76:
// return after this one. That is, it reports whether another call to
I understand the confusion. […]
I've taken your first sentence, except s/reports/indicates/. My impression is the idiomatic use of these words is "[function] reports" and "[value] indicates", and brief randomized review of our public docs supports this understanding.
I don't think your second and third sentences address my initial concern. I want to highlight specifically that the API does not conform to the standard T,ok idiom to discourage users from mistakenly thinking it does.
They also seem to still fall under your original "don't add anything to the first sentence" objection. E.g., returning a zero Frame is already explained in the first paragraph. Users interested in manually decoding stack frame traces also seem very likely to already be aware they can make use of values returned by functions, including in control flow conditionals.
I've revised the "does not" wording to hopefully be punchier, and consistent with other "It does not" sentences that appear in Go's public docs. (With the caveat that "That is," appears far more often.)
I've instead tried to incorporate your second two sentences by expanding the Frames example and including an explicit reference to that, like we occasionally do elsewhere. (I had initially missed the example, because navigating to #CallersFrames puts both CallersFrames and Frames.Next on screen, but the example is just above under Frames instead. But also, expanding examples on golang.org just fails about 50% of the time for me anyway, so I often skip looking at them unless I specifically think it'll be useful enough to justify refreshing the page.)
PTAL
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Matthew Dempsky.
Patch set 3:Code-Review +2
1 comment:
Patchset:
Thanks.
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Sam Mortimer (Gerrit)
Attention is currently required from: Matthew Dempsky.
1 comment:
File src/runtime/symtab.go:
It does not necessarily indicate whether this call
// returned one.This suggests that callers can sometimes use more to determine whether the current call returns a valid frame. If this is not your intention, I would head this off by removing this sentence altogether.
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Dempsky (Gerrit)
Attention is currently required from: Sam Mortimer.
Patch set 3:Trust +1
1 comment:
File src/runtime/symtab.go:
It does not necessarily indicate whether this call
// returned one.
This suggests that callers can sometimes use more to determine whether the current call returns a va […]
No.
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Dempsky (Gerrit)
Matthew Dempsky submitted this change.
runtime: clarify Frames.Next documentation
I wrote code that relied on this API, but I misunderstood the original
description of the "more" result. As a consequence, my code always
stopped one frame early.
This CL expands the documentation to be more explicit and specifically
call out my confusion (i.e., that the "more" result indicates whether
the *next* Next call will return a valid Frame, and not whether this
call did).
Change-Id: If135f8f8c05425073d45377c4179e4f79e6bd6ca
Reviewed-on: https://go-review.googlesource.com/c/go/+/329389
Run-TryBot: Matthew Dempsky <mdem...@google.com>
TryBot-Result: Go Bot <go...@golang.org>
Reviewed-by: Ian Lance Taylor <ia...@golang.org>
Trust: Matthew Dempsky <mdem...@google.com>
---
M src/runtime/example_test.go
M src/runtime/symtab.go
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/src/runtime/example_test.go b/src/runtime/example_test.go
index e4912a5..dcb8f77 100644
--- a/src/runtime/example_test.go
+++ b/src/runtime/example_test.go
@@ -12,12 +12,15 @@
func ExampleFrames() {
c := func() {
- // Ask runtime.Callers for up to 10 pcs, including runtime.Callers itself.
+ // Ask runtime.Callers for up to 10 PCs, including runtime.Callers itself.
pc := make([]uintptr, 10)
n := runtime.Callers(0, pc)
if n == 0 {
- // No pcs available. Stop now.
- // This can happen if the first argument to runtime.Callers is large.
+ // No PCs available. This can happen if the first argument to
+ // runtime.Callers is large.
+ //
+ // Return now to avoid processing the zero Frame that would
+ // otherwise be returned by frames.Next below.
return
}
@@ -25,9 +28,12 @@
frames := runtime.CallersFrames(pc)
// Loop to get frames.
- // A fixed number of pcs can expand to an indefinite number of Frames.
+ // A fixed number of PCs can expand to an indefinite number of Frames.
for {
frame, more := frames.Next()
+
+ // Process this frame.
+ //
// To keep this example's output stable
// even if there are changes in the testing package,
// stop unwinding when we leave package runtime.
@@ -35,6 +41,8 @@
break
}
fmt.Printf("- more:%v | %s\n", more, frame.Function)
+
+ // Check whether there are more frames to process after this one.
if !more {
break
}
diff --git a/src/runtime/symtab.go b/src/runtime/symtab.go
index 6b535dfc..999300a 100644
--- a/src/runtime/symtab.go
+++ b/src/runtime/symtab.go
@@ -68,8 +68,15 @@
return f
}
-// Next returns frame information for the next caller.
-// If more is false, there are no more callers (the Frame value is valid).
+// Next returns a Frame representing the next call frame in the slice
+// of PC values. If it has already returned all call frames, Next
+// returns a zero Frame.
+//
+// The more result indicates whether the next call to Next will return
+// a valid Frame. It does not necessarily indicate whether this call
+// returned one.
+//
+// See the Frames example for idiomatic usage.
func (ci *Frames) Next() (frame Frame, more bool) {
for len(ci.frames) < 2 {
// Find the next frame.
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Dempsky (Gerrit)
1 comment:
File src/runtime/symtab.go:
It does not necessarily indicate whether this call
// returned one.No.
To elaborate: I spent hours nitpicking the wording of these 3 sentences with Ian, and the entire point of this CL was to add an explicit warning that "more" does not indicate the validity of the "frame" returned by *this* call, which you now suggest removing again.
I intended this CL to be a quick doc improvement based on recent, personal experience I had misinterpreting the previous documentation that led to misuse of the API. But instead it turned into a full-day effort, distracting me from Go 1.17 release-blocking issues and progress on generics for Go 1.18. I also now feel less enthusiastic about mailing future CLs to improve documentation when I run into issues as a user.
I'm sure your comment was well intended. But in context, I found it to be dismissive and thus frustrating. It did not demonstrate to me an understanding of the original issue at hand or the issues that were discussed earlier during review.
I think having high-quality documentation is very important for Go. If you think revising the wording further helps with that, that sounds great. I encourage you to work on that. Please send a followup CL to Ian. Thanks.
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Matthew Dempsky.
1 comment:
File src/runtime/symtab.go:
It does not necessarily indicate whether this call
// returned one.
To elaborate: I spent hours nitpicking the wording of these 3 sentences with Ian, and the entire poi […]
I'm sorry you found the process so frustrating. If there is anything I can do to make it easier, please let me know.
To view, visit change 329389. To unsubscribe, or for help writing mail filters, visit settings.