Rewrite docs for fetch_update for clarity by hkBst · Pull Request #136036 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
In particular also uses the same names for the orderings as compare_exchange does to reduce confusion as per #89116.
r? @thomcc
rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
This comment has been minimized.
Once the proposed text replacement is approved, I will copy it to the other two places that define fetch_update.
In particular also uses the same names for the orderings as compare_exchange does to reduce confusion as per rust-lang#89116.
| mut f: F) -> Result<$int_type, $int_type> |
|---|
| where F: FnMut($int_type) -> Option<$int_type> { |
| let mut prev = self.load(fetch_order); |
| let mut prev = self.load(failure); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps a comment explaining why we are loading with the 'failure' order could be added here?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sold on this particular naming, but this change tries to bring this into line with compare_exchange's use of the terms. It explains the terms like this: "success describes the required ordering for the read-modify-write operation that takes place if the comparison with current succeeds. failure describes the required ordering for the load operation that takes place when the comparison fails.". Perhaps update_order and load_order are better terms?
Comment on lines +3119 to -3135
| /// Note: susceptible to the [ABA Problem](https://en.wikipedia.org/wiki/ABA\_problem). |
|---|
| /// |
| /// **Note**: This method is only available on platforms that support atomic operations on |
| #[doc = concat!("[`", $s_int_type, "`].")] |
| /// |
| /// # Considerations |
| /// |
| /// This method is not magic; it is not provided by the hardware. |
| /// It is implemented in terms of |
| #[doc = concat!("[`", stringify!($atomic_type), "::compare_exchange_weak`],")] |
| /// and suffers from the same drawbacks. |
| /// In particular, this method will not circumvent the [ABA Problem]. |
| /// |
| /// [ABA Problem]: https://en.wikipedia.org/wiki/ABA\_problem |
| /// |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this changed? IMO the longer version is better.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation using compare_exchange_weak is now explicitly described. This makes it unnecessary to say it is not magic, because that is now obvious. Also, users are more likely to look at the docs for compare_exchange_weak. That leaves the "in particular ... ABA problem", which I've shortened to its core, which is that this method is "susceptible to the ABA Problem".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explicit description of how the implementation uses compare_exchange_weak is also why I removed the copy of the description of the constraints on the memory orderings which comes directly from using compare_exchange_weak. Thanks for reviewing!
Labels
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.