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
Document become
keyword
#113095
base: master
Are you sure you want to change the base?
Document become
keyword
#113095
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
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.
A few small nits.
The job Click to see the possible cause of the failure (guessed by this bot)
|
/// is part of a recursive cycle in the call graph. | ||
/// | ||
/// For example note that the functions `halt` and `halt_loop` below are | ||
/// identical, they both do nothing, forever. However, `stack_overflow` is |
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.
/// identical, they both do nothing, forever. However, `stack_overflow` is | |
/// identical: they both do nothing, forever. However, `stack_overflow` is |
/// | ||
/// For example note that the functions `halt` and `halt_loop` below are | ||
/// identical, they both do nothing, forever. However, `stack_overflow` is | ||
/// different from them, even though it is written almost identically to |
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.
/// different from them, even though it is written almost identically to | |
/// different from them. Even though it is written almost identically to |
/// For example note that the functions `halt` and `halt_loop` below are | ||
/// identical, they both do nothing, forever. However, `stack_overflow` is | ||
/// different from them, even though it is written almost identically to | ||
/// `halt`, `stack_overflow` exhausts the stack and so causes a stack | ||
/// overflow, instead of running forever. | ||
/// | ||
/// | ||
/// ``` | ||
/// #![feature(explicit_tail_calls)] | ||
/// | ||
/// # #[allow(unreachable_code)] | ||
/// fn halt() -> ! { | ||
/// become halt() | ||
/// } | ||
/// | ||
/// fn halt_loop() -> ! { | ||
/// loop {} | ||
/// } | ||
/// | ||
/// # #[allow(unconditional_recursion)] | ||
/// fn stack_overflow() -> ! { | ||
/// stack_overflow() // implicit return | ||
/// } | ||
/// ``` |
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.
This discusses a function that is "obviously wrong", which means it does not make it clear why one wants to use become
in "real" code. I think we can do slightly better than this, as the documentation should focus on improving the good cases, like e.g. writing "natural" recursive merge-sorts. The example improvement can still be contrived, however.
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.
That makes sense, hmm. I guess the problem (similarly to the discussions on the RFC) is that there is no concise example where using tail calls makes sense in rust — most, if not all, small examples can be written just as good with a loop.
Maybe it would make sense to have two examples? One a bit silly, maybe a slice fold, and the other longer one with something like an interpreter?
Reading it now I see that this is a bad example, but I'm not sure what example would be good.
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.
A silly fold would be good! I'm not looking for "a loop wouldn't be just as good", just something that actually feels like something a human would want to write.
/// For example note that the functions `halt` and `halt_loop` below are | ||
/// identical, they both do nothing, forever. However, `stack_overflow` is | ||
/// different from them, even though it is written almost identically to | ||
/// `halt`, `stack_overflow` exhausts the stack and so causes a stack | ||
/// overflow, instead of running forever. |
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.
Isn't LLVM allowed to optimize stack_overflow()
into loop { }
? I know we don't allow it to remove infinite loops, but...
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.
It is allowed, but it also is allowed not to.
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.
Yeah. I guess it's allowed to do it in all these cases, right?
I guess what I'm concerned about is the example being so trivial that it doesn't hold up to even trivial examination.
// | ||
/// Perform a tail-call of a function. | ||
/// | ||
/// A `become` transfers the execution flow to a function in such a way, that |
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.
/// A `become` transfers the execution flow to a function in such a way, that | |
/// A `become` transfers the execution flow to a function in such a way that |
Unsure about the direction of this documentation focusing on the absurd case. @rustbot author |
@WaffleLapkin |
@JohnCSimon this is blocked on actually implementing the feature (it would be silly to document a feature one can't use). Also I need to address the review comments above. Note for future self: example of |
The feature is not yet implemented, so I'm not sure if we should merge this right away, promoting an incomplete feature is probably not the best idea. But the docs can be reviewed while the implementation work is being done.