Fix wrong statement in compare_exchange doc #34046
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
cc @Amanieu. Overall looks fine to me. |
|
Nice catch! This sentence could probably be added to |
|
It looks fine to me too, but since atomics are an area where I'm not that strong, a double-check by @rust-lang/libs would be nice, given this is about guarantees. |
|
@steveklabnik FYI, @Amanieu can definitely head up the technical review here (as a concurrency expert). In any case, LGTM as well. I might even suggest nixing "this value" and just repeating "the return value" for maximum clarity. |
|
This is one of those modules I haven't gone over yet, so I'm fine merging without the prose being absolutely perfect. Thanks all! @bors: r+ rollup |
|
📌 Commit 8841f26 has been approved by |
Fix wrong statement in compare_exchange doc
The documentation for `core::sync::atomic::AtomicSomething::compare_exchange` contains a wrong, or imprecise, statement about the return value. It goes:
The return value is a result indicating whether the new value was written and containing
the previous value. On success this value is guaranteed to be equal to `new`.
In the second sentence, `this value` is gramatically understood as referring to `return value` from the first sentence. Due to how CAS works, the returned value is always what was in the atomic variable _before_ the operation occurred, not what was written into it during the operation. Hence, the fixed doc should say:
The return value is a result indicating whether the new value was written and containing
the previous value. On success this value is guaranteed to be equal to `current`.
This version is confirmed by the runnable examples in variants of `AtomicSomething`, e.g.
assert_eq!(some_bool.compare_exchange(true, false, Ordering::Acquire, Ordering::Relaxed),
Ok(true));
where the returned value is `Ok(current)`. This PR fixes all occurrences of this bug I could find.
An alternative solution would be to modify the second sentence so that it refers to the value _written_ into the Atomic rather than what was there before, in which case it would be correct. Example alternative formulation:
On success the value written into the `bool`/`usize`/`whatever` is guaranteed to be equal to `new`.
r? @steveklabnik
Fix wrong statement in compare_exchange doc
The documentation for `core::sync::atomic::AtomicSomething::compare_exchange` contains a wrong, or imprecise, statement about the return value. It goes:
The return value is a result indicating whether the new value was written and containing
the previous value. On success this value is guaranteed to be equal to `new`.
In the second sentence, `this value` is gramatically understood as referring to `return value` from the first sentence. Due to how CAS works, the returned value is always what was in the atomic variable _before_ the operation occurred, not what was written into it during the operation. Hence, the fixed doc should say:
The return value is a result indicating whether the new value was written and containing
the previous value. On success this value is guaranteed to be equal to `current`.
This version is confirmed by the runnable examples in variants of `AtomicSomething`, e.g.
assert_eq!(some_bool.compare_exchange(true, false, Ordering::Acquire, Ordering::Relaxed),
Ok(true));
where the returned value is `Ok(current)`. This PR fixes all occurrences of this bug I could find.
An alternative solution would be to modify the second sentence so that it refers to the value _written_ into the Atomic rather than what was there before, in which case it would be correct. Example alternative formulation:
On success the value written into the `bool`/`usize`/`whatever` is guaranteed to be equal to `new`.
r? @steveklabnik
The documentation for
core::sync::atomic::AtomicSomething::compare_exchangecontains a wrong, or imprecise, statement about the return value. It goes:The return value is a result indicating whether the new value was written and containing
the previous value. On success this value is guaranteed to be equal to
new.In the second sentence,
this valueis gramatically understood as referring toreturn valuefrom the first sentence. Due to how CAS works, the returned value is always what was in the atomic variable before the operation occurred, not what was written into it during the operation. Hence, the fixed doc should say:The return value is a result indicating whether the new value was written and containing
the previous value. On success this value is guaranteed to be equal to
current.This version is confirmed by the runnable examples in variants of
AtomicSomething, e.g.where the returned value is
Ok(current). This PR fixes all occurrences of this bug I could find.An alternative solution would be to modify the second sentence so that it refers to the value written into the Atomic rather than what was there before, in which case it would be correct. Example alternative formulation:
On success the value written into the
bool/usize/whateveris guaranteed to be equal tonew.r? @steveklabnik