std: edit [T]::swap docs

[marcospb19]

Add a note about what happens when index arguments are equal.

[rustbot]

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

[clubby789]

Is this true? Reading the code (and on godbolt) there does not appear to be any special-case for equality.

[marcospb19]

Is this true?

From local testing it looks like it is, otherwise, I can add a check.

pub const fn swap(&mut self, a: usize, b: usize) {
    if a == b {
        return;
    }
    ...

Well... We'd need to think if somebody might be relying on it's weird behavior, in case it has one.

(and on godbolt)

There's optimization going on (I called v.swap(0, 0) and it became no-op assembly, just ret), I believe, you need to pass a and b to std::hint::black_box before feeding them into Vec::swap, if you're willing to analyze the compiler output.

[marcospb19]

Confirming, it is true by the underlying implementation of ptr::swap.

pub const unsafe fn swap<T>(x: *mut T, y: *mut T) {
    // Give ourselves some scratch space to work with.
    // We do not have to worry about drops: `MaybeUninit` does nothing when dropped.
    let mut tmp = MaybeUninit::<T>::uninit();

    // Perform the swap
    // SAFETY: the caller must guarantee that `x` and `y` are
    // valid for writes and properly aligned. `tmp` cannot be
    // overlapping either `x` or `y` because `tmp` was just allocated
    // on the stack as a separate allocated object.
    unsafe {
        copy_nonoverlapping(x, tmp.as_mut_ptr(), 1);
        copy(y, x, 1); // `x` and `y` may overlap
        copy_nonoverlapping(tmp.as_ptr(), y, 1);
    }
}

[clubby789]

Ah, I misunderstood (I thought you meant the values at a and b, rather than the indexes themselves). Still, ptr::swap does not seem to check if the indexes/pointers are the same - indeed, the call to copy explicitly mentions that they may overlap so copy_nonoverlapping may not be used.
Being a no-op is not guarenteed as far as I can tell, although it happens to be optimised out when the optimiser can see the two indees are the same i.e. this example will panic

[marcospb19]

@clubby789 I believe the snippet I sent you above shows that it is guaranteed, specially this part:

copy_nonoverlapping(x, tmp.as_mut_ptr(), 1);
copy(y, x, 1); // `x` and `y` may overlap
copy_nonoverlapping(tmp.as_ptr(), y, 1);

You can interpret them as

*tmp = *x;   // No overlap
*x = *y;    // Might overlap
*y = *tmp; // No overlap

Now assume x and y point to the same location x:

*tmp = *x; // (1)
*x = *x;   // (2)
*x = *tmp; // (3)

Assignment in 2 is never read, because it was overwritten by value in assignment 3, so you effectively have:

*tmp = *x; // (1)
*x = *tmp; // (3)

Which is guaranteed to work because tmp is a temporary stack-allocated buffer (impossible to overlap with the rest).

[clubby789]

This is not a no-op though - it's 3 moves (which can panic if OOB).

[marcospb19]

Oh, I'm sorry! By no-op I meant "it doesn't change anything, there is no mutation or swaps".

Do you suggest I change the comment like so?

-/// If `a` equals to `b`, this is a no-op.
+/// If `a` equals to `b`, no swaps are done.

[Mark-Simulacrum]

I don't think the implementation guarantees that no swaps are done. In debug mode we swap even with equal arguments: https://rust.godbolt.org/z/z174eW9eh

I think in most cases this is fine, I wouldn't add a check to the swap code to skip the check as it's probably true that the vast majority of swaps have non-equal arguments and the extra check is a wasted computation.

[marcospb19]

I edited the text accordingly, now it should make more sense?

[Mark-Simulacrum]

@bors r+ rollup

I guess this seems OK, though I'm not sure that it's very meaningful to add this to the docs.

[bors]

📌 Commit 30c61ee has been approved by Mark-Simulacrum

It is now in the queue for this repository.