Rewrite docs for fetch_update for clarity #136036
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
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. |
|
☔ The latest upstream changes (presumably #136185) made this pull request unmergeable. Please resolve the merge conflicts. |
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); |
There was a problem hiding this comment.
perhaps a comment explaining why we are loading with the 'failure' order could be added here?
There was a problem hiding this comment.
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?
| /// 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 | ||
| /// |
There was a problem hiding this comment.
Why was this changed? IMO the longer version is better.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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!
In particular also uses the same names for the orderings as compare_exchange does to reduce confusion as per #89116.