Skip to content

Conversation

Pyr0de
Copy link
Contributor

@Pyr0de Pyr0de commented Jan 30, 2025

Changed

  • set / initialize / full to initialized state
  • uninitialize / empty to uninitialized state
  • f to f()
  • Added explaination of uninitialized state & initialized state

OnceCell Docs
OnceLock Docs

Fixes #85716
@rustbot label +A-docs

@rustbot
Copy link
Collaborator

rustbot commented Jan 30, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ChrisDenton (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools labels Jan 30, 2025
@tgross35
Copy link
Contributor

Fwiw I think "initialized" and "uninitialized" is the better terminology here. "Initialized" makes it clear that it is a one-time occurrence, where as set/empty sounds more like Option where you can go back and forth.

@hkBst
Copy link
Member

hkBst commented Jan 30, 2025

@tgross35 Funny that you should mention Option. It really is like Option in that respect and can go back to being unset (to use the proposed new term).

@tgross35
Copy link
Contributor

There is API to unset these types, but the main problem they solve is initialization behind a shared &T reference with get_or_init (which has "initialize" in the name), rather than &mut T (if you always have &mut T, you could just use an Option). The changes "full" -> "set" seem reasonable here, but I am unconvinced that "initialize" -> "set" is an improvement.

@hkBst
Copy link
Member

hkBst commented Jan 30, 2025

There is API to unset these types, but the main problem they solve is initialization behind a shared &T reference with get_or_init (which has "initialize" in the name), rather than &mut T (if you always have &mut T, you could just use an Option). The changes "full" -> "set" seem reasonable here, but I am unconvinced that "initialize" -> "set" is an improvement.

You do have a point there! Fundamentally there are 3 things which need a name that can be used consistently. There is the empty/unset/uninitialized state, the full/set/initialized state, and the transition set/initialize:

  • Originally I liked empty ---initialize---> initialized
  • @Pyr0de's PR does empty ---set---> set
  • you seem to like uninitialized ---initialize---> initialized
  • another option is unset ---initialize---> initialized(or set)

@ChrisDenton
Copy link
Member

ChrisDenton commented Jan 30, 2025

r? tgross35

Since tgross35 has started but feel free to flip it back to me if you'd prefer.

Edit: I used the wrong command initially, sorry! Corrected now.

@bors

This comment was marked as outdated.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 30, 2025
@rustbot rustbot assigned tgross35 and unassigned ChrisDenton Jan 30, 2025
@ChrisDenton

This comment was marked as outdated.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 30, 2025
@Pyr0de
Copy link
Contributor Author

Pyr0de commented Feb 1, 2025

I agree with @hkBst's idea of empty-initialize-initialized

but empty-set-initialized could be used to include set

I think both are good options

@hkBst
Copy link
Member

hkBst commented Feb 1, 2025

Actually, I think @tgross35 has convinced me that uninitialized ---initialize---> initialized is the way to go, and it goes with most of the methods that end in init. To solve the issue of confusion with uninitialized memory, we could explicitly state near the beginning what the two states are called...

@Pyr0de
Copy link
Contributor Author

Pyr0de commented Feb 1, 2025

To solve the issue of confusion with uninitialized memory, we could explicitly state near the beginning what the two states are called...

Could you suggest some way to show this information?

@hkBst
Copy link
Member

hkBst commented Feb 1, 2025

Could you suggest some way to show this information?

Maybe something like: A OnceCell conceptually has two states, called the uninitialized state and the initialized state. Like an enum {Uninitialized, Initialized(T)}, except that it has invariants to uphold, so the enum is hidden.

That would do the trick I think.

@Pyr0de Pyr0de changed the title OnceCell & OnceLock docs: Using set/empty consistently OnceCell & OnceLock docs: Using (un)initialized consistently Feb 2, 2025
@Pyr0de
Copy link
Contributor Author

Pyr0de commented Feb 2, 2025

@hkBst @tgross35
I have made some changes and edited the original description to reflect the changes

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 2, 2025
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a handful of places where the wording can be simplified, plus a slight terminology adjustment to be more consistent with the other cell types. But the rest of the wording looks better to me.

@Pyr0de Pyr0de requested a review from tgross35 February 3, 2025 09:51
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nits then r=me

@tgross35
Copy link
Contributor

tgross35 commented Feb 3, 2025

Thanks for all the work getting this through!

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Feb 3, 2025

📌 Commit f8b01b3 has been approved by tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 3, 2025
@bors bors merged commit f2b7a29 into rust-lang:master Feb 4, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 4, 2025
@hkBst
Copy link
Member

hkBst commented Feb 4, 2025

Great work @tgross35 and @Pyr0de !

@Pyr0de
Copy link
Contributor Author

Pyr0de commented Feb 4, 2025

Thanks for all the help @hkBst and @tgross35!

@Pyr0de Pyr0de deleted the oncecell-docs branch February 22, 2025 16:35
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 S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OnceCell docs: empty/full terminology v.s. (un)initialized

6 participants