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?

Goth

Doesn't compile on GCC or Clang, but does compile on MSVC 2015. Another undocumented feature perhaps. – Yang

Damn, those are some good ratios within less than the first hour. +16 upvotes, viewed 77 times, and 4 favorites. – Antlia

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. – Darrin

@Darrin foo is a layer of indirection we use over the standard library. Many of the platforms we support have incomplete/buggy standard libraries, and on those platforms we will do things like direct foo::shared_ptr to boost::shared_ptr instead of std::shared_ptr. One of our platforms has an old std::swap() that copies instead of moves, so we provided a replacement in foo. – Goth

@TavianBarnes I've updated my answer in response to both your comments. – Pedestal

Semi-unrelated: Andrew Koenig did not invent Argument Dependent Lookup. – Gyral

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.

Phenanthrene

You can find a simpler breakdown here for those that don't speak lawyer. Anyways, while libstdc++ is certainly following the rules here there doesn't seem to be anything that required it to use ADL swap inside reset, considering that libcxx's swap of the deleters is functionally equivalent. I'll chalk this up to quality of implementation. – Pedestal

@Pedestal it's QoI on the OP's part here. Just don't provide general templates duplicating customization points of STL templates, and especially don't provide your own classes to such STL templates when you do. – Darrin

@Pedestal For fancy pointers that actually defined custom swaps, using those may well be more efficient. – Neuroglia

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.

Pedestal

"I would recommend using Boost instead of reinventing the wheel" Agreed! We do this for many things in foo. Unfortunately boost::swap() does not do the move-assignments, but just delegates to std::swap. – Goth

@TavianBarnes That may be true but I believe that the Boost equivalent of unique_ptr uses move semantics for swap for example. – Pedestal

Right, but I often want to do swap(a, b); where a and b are of a non-copyable type. It's a pain to declare an overload for every move-only type I define, so I need a swap() implementation that moves. – Goth

@TavianBarnes I don't usually give shady advice, but have you considered replacing swap on the shared library level? I believe that's common to do in embedded systems. – Pedestal

@Pedestal I updated your link to GitHub to the commit directly and ran it through GitHub's link shortener. Feel free to revert the edit if it's the wrong parts. – Distefano

Answer 3

This technique can be used to avoid foo::swap() getting found by ADL:

namespace foo
{
    namespace adl_barrier
    {
        template <typename T>
        void swap(T& a, T& b)
        {
            T tmp = std::move(a);
            a = std::move(b);
            b = std::move(tmp);
        }
    }

    using namespace adl_barrier;
}

This is how Boost.Range's free-standing begin()/end() functions are defined. I tried something similar before asking the question, but did using adl_barrier::swap; instead, which doesn't work.

As for whether the snippet in the question should work as-is, I'm not sure. One complication I can see is that unique_ptr can have custom pointer types from the Deleter, which should be swapped with the usual using std::swap; swap(a, b); idiom. That idiom is clearly broken for foo::bar* in the question.

Goth

If it's "totally general", then why don't you just import std::swap into foo with a using-declaration? – Phenanthrene

@Phenanthrene foo::swap() was written because one of our platforms has a standard library with a deficient std::swap() (does copies rather than moves). I will probably do using std::swap; except on that platform. – Goth

btw, I think it's better to ADL-protect your type (i.e. foo::bar) instead of your functions (i.e. foo::swap), since those are the ones causing the grief, and the number of functions in namespace foo is open-ended (someone might add foo::begin and foo::end someday and then you have new trouble, ADL-protected foo::bar guards against that). – Darrin