compare_and_swap "migration" documentation should explain how to get return value

[RalfJung]

The compare_and_swap docs explain how to switch the order to compare_exchange, but that is not the only difference: the return type also changed. The docs don't say anything about how to best get the old behavior back with the new method. Many concurrent algorithms are written based on getting the old value back, so there should be a convenient way to get that behavior.

Cc @faern

[RalfJung]

FWIW, the underlying primitive in LLVM that backs compare_exchange returns (T, bool) -- which IMO makes much more sense than Result<T, T>. But that ship has long sailed...

[RalfJung]

Looks like unwrap_or_else(|x| x) is one option. It's not pretty but it works.

[m-ou-se]

related: #79315

[lolgesten]

I'm probably dense, but I don't understand how I am supposed to change my code.

This is what I had, but reading the compare_exchange documentation, I don't understand what I'm supposed to do now.

    if !DRAIN_MODE.compare_and_swap(false, true, Ordering::Relaxed) {

I understand the motivation for the deprecation, but I feel sad it's done at the cost of a more complex API. For my use case, I simply want atomic access to a bool, and now I'm forced to get my head around the double Ordering parameters to get slightly better performance on ARM (a platform I don't use) + a double boolean Result return type.

Wouldn't it be possible to not deprecate compare_and_swap for people like me, who just want the simple use case?

[RalfJung]

@lolgesten without commenting on the rest of what you said, this is how you could change your code:

    if !DRAIN_MODE.compare_exchange(false, true, Ordering::Relaxed, Ordering::Relaxed).unwrap_or_else(|x| x) {

This translation can be applied systematically.

However, I assume what you really want to express here is "if the CAS happened successfully", which would be more clearly expressed as

    if let Ok(_) = DRAIN_MODE.compare_exchange(false, true, Ordering::Relaxed, Ordering::Relaxed) {

or

    if DRAIN_MODE.compare_exchange(false, true, Ordering::Relaxed, Ordering::Relaxed).is_ok() {

Arguably, this code is more clear than your original code, since it is easier to see that it checks for "successful CAS".

[lolgesten]

@RalfJung thanks! This is what I was suspecting I should do.

In terms of improving the documentation. The table added in the doc for compare_and_swap has the column names "Original", "Success", "Failure". Maybe that could be improved to more clearly indicate the columns are for compare_and_swap and compare_exchange arg1 and compare_exchange arg2.

And also

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.

It's not clear to me whether the doc about failure here is equivalent to Result::Err(boolean). The docs could maybe be clearer about what Result::Ok vs Result:Err means.

[faern]

I agree that the "Original" column could be better named. But the column names "Success" and "Failure" are named after the arguments to compare_exchange. I think that makes sense. arg1 and arg2 would be strange since the first two arguments to compare_exchange is current and new. But yes, the text could elaborate a bit more and show one example of a migration maybe.

[RalfJung]

Can they be simply done by opening a PR

Yes. :-)

[hkBst]

@RalfJung do you still feel that the compare_exchange docs need to mention some way of unpacking the Result to a value? In your own example you seemed to prefer if let Ok() = and if .. .is_ok().