Is it okay to define a totally general swap() function?

Question

The following snippet:

#include <memory> #include <utility>  namespace foo {     template <typename T>     void swap(T& a, T& b)     {         T tmp = std::move(a);         a = std::move(b);         b = std::move(tmp);     }      struct bar { }; }  void baz() {     std::unique_ptr<foo::bar> ptr;     ptr.reset(); } 

does not compile for me:

$ g++ -std=c++11 -c foo.cpp In file included from /usr/include/c++/5.3.0/memory:81:0,                  from foo.cpp:1: /usr/include/c++/5.3.0/bits/unique_ptr.h: In instantiation of ‘void std::unique_ptr<_Tp, _Dp>::reset(std::unique_ptr<_Tp, _Dp>::pointer) [with _Tp = foo::bar; _Dp = std::default_delete<foo::bar>; std::unique_ptr<_Tp, _Dp>::pointer = foo::bar*]’: foo.cpp:20:15:   required from here /usr/include/c++/5.3.0/bits/unique_ptr.h:342:6: error: call of overloaded ‘swap(foo::bar*&, foo::bar*&)’ is ambiguous   swap(std::get<0>(_M_t), __p);       ^ In file included from /usr/include/c++/5.3.0/bits/stl_pair.h:59:0,                  from /usr/include/c++/5.3.0/bits/stl_algobase.h:64,                  from /usr/include/c++/5.3.0/memory:62,                  from foo.cpp:1: /usr/include/c++/5.3.0/bits/move.h:176:5: note: candidate: void std::swap(_Tp&, _Tp&) [with _Tp = foo::bar*]      swap(_Tp& __a, _Tp& __b)      ^ foo.cpp:7:10: note: candidate: void foo::swap(T&, T&) [with T = foo::bar*]      void swap(T& a, T& b) 

Is this my fault for declaring a swap() function so general that it conflicts with std::swap?

If so, is there a way to define foo::swap() so that it doesn't get hauled in by Koenig lookup?

Answer 1

• unique_ptr<T> requires T* to be a NullablePointer [unique.ptr]p3
• NullablePointer requires lvalues of T* to be Swappable [nullablepointer.requirements]p1
• Swappable essentially requires using std::swap; swap(x, y); to select an overload for x, y being lvalues of type T* [swappable.requirements]p3

In the last step, your type foo::bar produces an ambiguity and therefore violates the requirements of unique_ptr. libstdc++'s implementation is conforming, although I'd say this is rather surprising.

---

The wording is of course a bit more convoluted, because it is generic.

[unique.ptr]p3

If the type remove_reference_t<D>::pointer exists, then unique_ptr<T, D>::pointer shall be a synonym for remove_reference_t<D>::pointer. Otherwise unique_ptr<T, D>::pointer shall be a synonym for T*. The type unique_ptr<T, D>::pointer shall satisfy the requirements of NullablePointer.

(emphasis mine)

[nullablepointer.requirements]p1

A NullablePointer type is a pointer-like type that supports null values. A type P meets the requirements of NullablePointer if:

• [...]
• lvalues of type P are swappable (17.6.3.2),
• [...]

[swappable.requirements]p2

An object t is swappable with an object u if and only if:

• the expressions swap(t, u) and swap(u, t) are valid when evaluated in the context described below, and
• [...]

[swappable.requirements]p3

The context in which swap(t, u) and swap(u, t) are evaluated shall ensure that a binary non-member function named “swap” is selected via overload resolution on a candidate set that includes:

• the two swap function templates defined in <utility> and
• the lookup set produced by argument-dependent lookup.

Note that for a pointer type T*, for purposes of ADL, the associated namespaces and classes are derived from the type T. Hence, foo::bar* has foo as an associated namespace. ADL for swap(x, y) where either x or y is a foo::bar* will therefore find foo::swap.

Answer 2

The problem is libstdc++'s implementation of unique_ptr. This is from their 4.9.2 branch:

https://gcc.gnu.org/onlinedocs/gcc-4.9.2/libstdc++/api/a01298_source.html#l00339

  338       void   339       reset(pointer __p = pointer()) noexcept   340       {   341     using std::swap;   342     swap(std::get<0>(_M_t), __p);   343     if (__p != pointer())   344       get_deleter()(__p);   345       } 

As you can see, there is an unqualified swap call. Now let's see libcxx (libc++)'s implementation:

https://git.io/vKzhF

_LIBCPP_INLINE_VISIBILITY void reset(pointer __p = pointer()) _NOEXCEPT {     pointer __tmp = __ptr_.first();     __ptr_.first() = __p;     if (__tmp)         __ptr_.second()(__tmp); }  _LIBCPP_INLINE_VISIBILITY void swap(unique_ptr& __u) _NOEXCEPT     {__ptr_.swap(__u.__ptr_);} 

They don't call swap inside reset nor do they use an unqualified swap call.

---

Dyp's answer provides a pretty solid breakdown on why libstdc++ is conforming but also why your code will break whenever swap is required to be called by the standard library. To quote TemplateRex:

You should have no reason to define such a general swap template in a very specific namespace containing only specific types. Just define a non-template swap overload for foo::bar. Leave general swapping to std::swap, and only provide specific overloads. source

As an example, this won't compile:

std::vector<foo::bar> v; std::vector<foo::bar>().swap(v); 

If you're targeting a platform with an old standard library/GCC (like CentOS), I would recommend using Boost instead of reinventing the wheel to avoid pitfalls like this.