-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add documentation for CallStack "caller" method #4702
Conversation
Updated per 2nd round of feedback. Keep it coming :-) |
Those 5 lines additions will be perfect for your first contribution 😆 |
I should have read the doc guidelines a bit more thoroughly but I also considered that this might be a ritual hazing for first time contributor to the repo? ;-) |
In all honestly, having this method at all is a bad idea because it encourages people to reply on stacktraces to be accurate. And in release mode, they absolutely are not. There is no way to tell which function you were called from in release mode. |
@RX14 That's right, I think it should at least be mentioned more clearly in the docs that the method is almost useless in |
Ok - can someone suggest a sentence or two with a possible hint as to why this is the case? i.e. "Note: In release mode, the usefulness of the callstack is questionable due to blah blah blah"
Whether this method should exist at all may be true but is probably beyond the scope of this 5 line code doc PR. |
@pglombardo it may be beyond the scope of this PR but there's little point landing this PR if we have a consensus that the method should be removed. |
That's true... what's the use case for |
If we're speaking about what the best API approach would be for this functionality, I would think that a class method on
The alternative is to call I can close this PR and make a separate one with these changes (and the method docs) if no one objects. |
CallStack is internal implementation detail, and musn't be documented publicly.
Release mode may inline calls, just like development inlines blocks (non captured ones). Backtrackes are never accurate, but still helpful and never wrong, whatever the optimizations. |
@@ -17,6 +17,10 @@ require "callstack/lib_unwind" | |||
require "debug/dwarf" | |||
{% end %} | |||
|
|||
# Returns the execution stack (a backtrace) of the current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per @ysbaddaden's comment, how about we clarify that this is a human readable execution stack and add a note that it can be used while debugging. Then I think it's ready to merge.
I think we still want this, in case anybody wants to pick it up and finish :) |
Resolved by #9076 |
Per convo in gitter, it would be helpful to have this method (
caller
) exposed in the API documentation.This PR adds a short method explanation and adds
callstack.cr
to the documentation build process.