1652365 - std::swap overloads are UB

Status: NEW

(Core :: MFBT, defect)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

In bug 1609996, some overloads of mozilla::Swap were replaced with overloads of std::swap in Atomics.h, Pair.h, and UniquePtr.h.

However, such overloads are explicitly undefined in C++, see http://eel.is/c++draft/namespace.std#1 :

Unless otherwise specified, the behavior of a C++ program is undefined if it adds declarations or definitions to namespace std or to a namespace within namespace std.

(I've just learnt about this in https://quuxplusone.github.io/blog/2020/07/11/the-std-swap-two-step/ , other details in https://stackoverflow.com/questions/14402990/should-you-overload-swap-in-the-std-namespace )

So these overloads should instead be free functions in their arguments' namespace, which can be found through ADL.

Corresponding call sites may need to be changed to naked swaps, or two-step using std::swap; swap(...); in generic code.
But we may actually need to review all explicit std::swap calls, possibly in a separate bug(?)

Comment 1

[Simon Giesecke [:sg] [he/him]]

I wonder if this = delete is meaningful at all: https://searchfox.org/mozilla-central/rev/25d5a4443a7e13cfa58eff38f1faa5e69f0b170f/mfbt/Atomics.h#513

It would be implicitly unavailable if Atomic<T> were not assignable. But AFAIU it is assignable, and in that case it seems inconsistent to disallow swapping. But maybe it shouldn't be assignable in the first place.