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

{Rc,Arc}::new_cyclic documentation is insufficient #95672

Closed
jdahlstrom opened this issue Apr 5, 2022 · 9 comments · Fixed by #95843
Closed

{Rc,Arc}::new_cyclic documentation is insufficient #95672

jdahlstrom opened this issue Apr 5, 2022 · 9 comments · Fixed by #95843
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools

Comments

@jdahlstrom
Copy link

jdahlstrom commented Apr 5, 2022

As I reported in the i.r-l.o prerelease thread, the documentation of the new(ly stabilized) Rc and Arc method new_cyclic could really use some elaboration before the 1.60 release.

I'm quite familiar with refcounting, strong/weak refs, etc. but just from the doc, I have no idea what problem the new method solves exactly, when I should use it, why the closure is needed, how the closure argument seems to come from "thin air", and what the closure should return.

@Aloso
Copy link
Contributor

Aloso commented Apr 5, 2022

s/new_acyclic/new_cyclic

@jdahlstrom jdahlstrom changed the title {Rc,Arc}::new_acyclic documentation is insufficient {Rc,Arc}::new_cyclic documentation is insufficient Apr 5, 2022
@jdahlstrom
Copy link
Author

Argh, thanks. Could've sworn I specifically double-checked that I wrote "cyclic"…

@CAD97
Copy link
Contributor

CAD97 commented Apr 5, 2022

  • Problem the method solves: constructing a T which holds a Weak<T> pointer to itself.
  • When you should use it: when you need to do the above. Previously you'd construct with e.g. Weak::dangling and then mutate the value (via e.g. get_mut) to fix up the self references.
  • Where the closure argument comes from: essentially, the point of new_cyclic is that the place is allocated first, with strong=0, weak=1. The place has no data yet (you're constructing the value to place there), but is in the "zombie" state where it has weak references but no data.
  • Why the closure is needed: the place needs to be "finished" once the value is created. In theory, you could "recover" any zombie weak, but for the most case, Rust wants to pretend that a "zombie" Weak and a dangling Weak are equivalent.
  • What the closure should return: the closure returns the object to place in the reference counted place. After you've constructed the object, the reference count is "activated" and the object is accessable by (clones of) the weak pointer given to the constructor closure.

For the most part, new_cyclic exists to construct enable_shared_from_this style objects more easily, which is what the example demonstrates. It can be used for other use cases, but this is the primary motivating example.

I'm biased because I know what new_cyclic is used for, so the existing documentation is enough. The docs aren't going to be changed for 1.60, but if you (or anyone) can improve the docs (with the above information or otherwise), PRs to do so are welcome!

@jethrogb
Copy link
Contributor

jethrogb commented Apr 5, 2022

I agree I had to read the documentation a few times to understand what this does.

@pietroalbini
Copy link
Member

Hi! Just a quick note that changing the documentation for the Rust 1.60.0 release itself will likely not be possible, that'd require a full release rebuild.

@GuillaumeGomez GuillaumeGomez added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Apr 9, 2022
@GuillaumeGomez
Copy link
Member

I'm sending a PR with explanations provided by @CAD97 so that it'll at least be in the next release.

@pietroalbini Why do you need a full release rebuild btw? Isn't it possible to regen the docs without rebuilding everything like the --keep-stage option on rustc?

@GuillaumeGomez
Copy link
Member

I opened #95843. Feedback is very welcome. :)

@pietroalbini
Copy link
Member

Why do you need a full release rebuild btw? Isn't it possible to regen the docs without rebuilding everything like the --keep-stage option on rustc?

Our release tooling only supports building everything at the same time.

@GuillaumeGomez
Copy link
Member

Well, at least it'll be present in the next release with a beta backport.

@bors bors closed this as completed in 3f07303 May 5, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
…u-se

Improve Rc::new_cyclic and Arc::new_cyclic documentation

Fixes rust-lang/rust#95672.

cc `@CAD97` (since I used your explanations)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants