Clarify &mut-methods' docs on sync::OnceLock #140715
Conversation
rustbot
added
S-waiting-on-review
|
I like how Mutex and RefCell phrase this: "since this method borrows the $thing mutably, ..". What about something similar like "since this method borrows the OnceLock mutably, no synchronization is necessary"? |
lukaslueg
force-pushed
the
oncecellsyncdocs
branch
from
031b2b8 to
767083a
Compare
library/std/src/sync/once_lock.rs
Outdated
| /// Returns `None` if the cell is uninitialized. | ||
| /// This method never blocks. Since it borrows the `OnceLock` mutably, | ||
| /// it is statically guaranteed that no active borrows to the `OnceLock` | ||
| /// or the underlying data exist, including from other threads. |
There was a problem hiding this comment.
Thank you for the fix here, sorry for the delay (I thought it was still waiting on a response to the previous comment). The change looks pretty good to me, a few possible tweaks:
- "Since it borrows" -> "Since this call borrows", to match mutex docs and disambiguate "it"
- Drop "or the underlying data" bit; since the
OnceLockowns data, that part is implied - If the "Returns
None..." sentence is meant to be its own paragraph, it needs an extra empty line between it and the start of the next. Or else just reflow to 100 chars.
There was a problem hiding this comment.
Nitpicking, yet I tend to disagree: "Since it borrows" refers to "This method". "This method never blocks. Since this call borrows..." is objectively worse imho. "Or the underlying data" is specifically there to drive home what would be implied otherwise; oftentimes &self-taking methods on some T return &Inner, while &mut self return nothing; but OnceLock (and others) make use of the fact that the very existence of &mut Inner implies that there is no usable &[mut] OnceLock; there can be no borrows of whats in the OnceLock if we can call these methods at all on the OnceLock. I'd like to avoid that very implication.
There was a problem hiding this comment.
Fair enough for "since it borrows", I missed that the previous sentence refers to "this method".
For the second bit, I suggested this change to align with the other similar types. There are plenty of times where ownership of the Mutex/RwLock/RefCell is discussed in their docs, but the term "underlying data" only shows up when the inner value is being accessed in some way. The motivation doesn't seem strong enough to update this everywhere that borrows on the containers are mentioned. (If Container<T> owns T and you have &mut Container<T>, nobody else can be borrowing the container so nobody else can access T).
lukaslueg
force-pushed
the
oncecellsyncdocs
branch
from
767083a to
3792ee4
Compare
This comment has been minimized.
This comment has been minimized.
|
Thanks! @bors r+ rollup |
bors
added
S-waiting-on-bors
lukaslueg
force-pushed
the
oncecellsyncdocs
branch
from
3792ee4 to
200d742
Compare
Was in the process of rebasing in case you are wondering. |
As in, on top of |
|
yes, unto recent |
|
Oh, yeah that's kind of an interesting message. It doesn't really matter for small PRs like this as long as there are no conflicts (the "compare" thing I mentioned above isn't either TBH), but 🤷. So feel free to rebase or not, should be fine either way :) |
|
I'm done here |
|
@tgross35, you'll need to r+ again, due to the forced push?! |
|
Sorry about that! @bors r+ rollup |
…ss35 Clarify &mut-methods' docs on sync::OnceLock Three small changes to the docs of `sync::OnceLock`: * The docs for `OnceLock::take()` used to [say](https://doc.rust-lang.org/std/sync/struct.OnceLock.html#method.take) "**Safety** is guaranteed by requiring a mutable reference." (emphasis mine). While technically correct, imho its not necessary to even mention safety - as opposed to unsafety - here: Safety never comes up wrt `OnceLock`, as there is (currently) no way to interact with a `OnceLock` in an unsafe way; there are no unsafe methods on `OnceLock`, so there is "safety" guarantee required anywhere. What we simply meant to say is "**Synchronization** is guaranteed...". * I've add that phrase to the other methods of `OnceLock` which take a `&mut self`, to highlight the fact that having a `&mut OnceLock` guarantees that synchronization with other threads is not required. This is the same as with [`Mutex::get_mut()`](https://doc.rust-lang.org/std/sync/struct.Mutex.html#method.get_mut), [`Cell::get_mut()`](https://doc.rust-lang.org/std/cell/struct.Cell.html#method.get_mut), and others. * In that spirit, the half-sentence "or being initialized" was removed from `get_mut()`, as there is no way that the `OnceLock` is being initialized while we are holding `&mut` to it. Probably a copy&paste from `.get()`
Rollup of 6 pull requests Successful merges: - #140715 (Clarify &mut-methods' docs on sync::OnceLock) - #141309 (x86 (32/64): go back to passing SIMD vectors by-ptr) - #141677 (Async drop - type instead of async drop fn, fixes #140484) - #141733 (C-variadic functions must be unsafe) - #141858 (Fix typo in `StructuralPartialEq` docs) - #141874 (add f16_epsilon and f128_epsilon diagnostic items) r? `@ghost` `@rustbot` modify labels: rollup
…ss35 Clarify &mut-methods' docs on sync::OnceLock Three small changes to the docs of `sync::OnceLock`: * The docs for `OnceLock::take()` used to [say](https://doc.rust-lang.org/std/sync/struct.OnceLock.html#method.take) "**Safety** is guaranteed by requiring a mutable reference." (emphasis mine). While technically correct, imho its not necessary to even mention safety - as opposed to unsafety - here: Safety never comes up wrt `OnceLock`, as there is (currently) no way to interact with a `OnceLock` in an unsafe way; there are no unsafe methods on `OnceLock`, so there is "safety" guarantee required anywhere. What we simply meant to say is "**Synchronization** is guaranteed...". * I've add that phrase to the other methods of `OnceLock` which take a `&mut self`, to highlight the fact that having a `&mut OnceLock` guarantees that synchronization with other threads is not required. This is the same as with [`Mutex::get_mut()`](https://doc.rust-lang.org/std/sync/struct.Mutex.html#method.get_mut), [`Cell::get_mut()`](https://doc.rust-lang.org/std/cell/struct.Cell.html#method.get_mut), and others. * In that spirit, the half-sentence "or being initialized" was removed from `get_mut()`, as there is no way that the `OnceLock` is being initialized while we are holding `&mut` to it. Probably a copy&paste from `.get()`
…ss35 Clarify &mut-methods' docs on sync::OnceLock Three small changes to the docs of `sync::OnceLock`: * The docs for `OnceLock::take()` used to [say](https://doc.rust-lang.org/std/sync/struct.OnceLock.html#method.take) "**Safety** is guaranteed by requiring a mutable reference." (emphasis mine). While technically correct, imho its not necessary to even mention safety - as opposed to unsafety - here: Safety never comes up wrt `OnceLock`, as there is (currently) no way to interact with a `OnceLock` in an unsafe way; there are no unsafe methods on `OnceLock`, so there is "safety" guarantee required anywhere. What we simply meant to say is "**Synchronization** is guaranteed...". * I've add that phrase to the other methods of `OnceLock` which take a `&mut self`, to highlight the fact that having a `&mut OnceLock` guarantees that synchronization with other threads is not required. This is the same as with [`Mutex::get_mut()`](https://doc.rust-lang.org/std/sync/struct.Mutex.html#method.get_mut), [`Cell::get_mut()`](https://doc.rust-lang.org/std/cell/struct.Cell.html#method.get_mut), and others. * In that spirit, the half-sentence "or being initialized" was removed from `get_mut()`, as there is no way that the `OnceLock` is being initialized while we are holding `&mut` to it. Probably a copy&paste from `.get()`
Rollup of 8 pull requests Successful merges: - #140715 (Clarify &mut-methods' docs on sync::OnceLock) - #141677 (Async drop - type instead of async drop fn, fixes #140484) - #141741 (Overhaul `UsePath`) - #141873 (Fixed a typo in `ManuallyDrop`'s doc) - #141876 (Don't declare variables in `ExprKind::Let` in invalid positions) - #141886 (Add missing 2015 edition directives) - #141889 (Add missing `dyn` keywords to tests that do not test for them) - #141891 (Fix borrowck mentioning a name from an external macro we (deliberately) don't save) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #140715 - lukaslueg:oncecellsyncdocs, r=tgross35 Clarify &mut-methods' docs on sync::OnceLock Three small changes to the docs of `sync::OnceLock`: * The docs for `OnceLock::take()` used to [say](https://doc.rust-lang.org/std/sync/struct.OnceLock.html#method.take) "**Safety** is guaranteed by requiring a mutable reference." (emphasis mine). While technically correct, imho its not necessary to even mention safety - as opposed to unsafety - here: Safety never comes up wrt `OnceLock`, as there is (currently) no way to interact with a `OnceLock` in an unsafe way; there are no unsafe methods on `OnceLock`, so there is "safety" guarantee required anywhere. What we simply meant to say is "**Synchronization** is guaranteed...". * I've add that phrase to the other methods of `OnceLock` which take a `&mut self`, to highlight the fact that having a `&mut OnceLock` guarantees that synchronization with other threads is not required. This is the same as with [`Mutex::get_mut()`](https://doc.rust-lang.org/std/sync/struct.Mutex.html#method.get_mut), [`Cell::get_mut()`](https://doc.rust-lang.org/std/cell/struct.Cell.html#method.get_mut), and others. * In that spirit, the half-sentence "or being initialized" was removed from `get_mut()`, as there is no way that the `OnceLock` is being initialized while we are holding `&mut` to it. Probably a copy&paste from `.get()`
Three small changes to the docs of
sync::OnceLock:OnceLock::take()used to say "Safety is guaranteed by requiring a mutable reference." (emphasis mine). While technically correct, imho its not necessary to even mention safety - as opposed to unsafety - here: Safety never comes up wrtOnceLock, as there is (currently) no way to interact with aOnceLockin an unsafe way; there are no unsafe methods onOnceLock, so there is "safety" guarantee required anywhere. What we simply meant to say is "Synchronization is guaranteed...".OnceLockwhich take a&mut self, to highlight the fact that having a&mut OnceLockguarantees that synchronization with other threads is not required. This is the same as withMutex::get_mut(),Cell::get_mut(), and others.get_mut(), as there is no way that theOnceLockis being initialized while we are holding&mutto it. Probably a copy&paste from.get()