[cfe-dev] Specializing std::swap

[Howard Hinnant]

On May 22, 2013, at 10:14 PM, Aleksandar Fabijanic <aleskx at gmail.com> wrote:

> Hi,
> 
> It was stated in this bug report
> 
> http://llvm.org/bugs/show_bug.cgi?id=10248
> 
> that specializing swap for non-std types in std namespace, although
> legal, is considered fragile. It is not entirely clear from the
> response why is that so.
> 
> Can someone shed some light on this?
> 
> Thanks in advance,
> 
> Alex


Although there is no good reason to specialized swap in std namespace, that was not the major problem.  The major problem was how swap was called:

	std::swap<Poco::Data::Statement>(s1, s2);

I.e. specifying the template argument.

This will match any std-defined swap with the following signature:

   template <class T> void swap(anything-at-all);

This causes the *signatures*, not the implementations of all of the following to be instantiated:

template <> void swap(__bit_reference<Poco::Data::Statement> __x, __bit_reference<Poco::Data::Statement> __y) _NOEXCEPT;
template <> void swap(shared_ptr< Poco::Data::Statement >& __x, shared_ptr< Poco::Data::Statement >& __y) _NOEXCEPT;
template <> void swap(weak_ptr<Poco::Data::Statement>& __x, weak_ptr<Poco::Data::Statement>& __y) _NOEXCEPT;
template <> void swap(unique_lock<Poco::Data::Statement>& __x, unique_lock<Poco::Data::Statement>& __y);

etc.

The first signature instantiation listed above is actually the trouble maker.  But the rest are questionable, especially the one involving unique_lock.  The template parameter for unique_lock should meet the BasicLockable requirements.  It is almost certain that Poco::Data::Statement is not a BasicLockable type.

The danger is that simply instantiating a function signature for swap can lead to a compile time error.  Maybe unique_lock<T> requires a typedef that Poco::Data::Statement doesn't have?  E.g: unique_lock<BasicLockable> requires BasicLockable::is_lock_free to exist.  This isn't the case.  But this is exactly what happened with __bit_reference<Poco::Data::Statement>:

/usr/include/c++/v1/__bit_reference:26:26: error: no type named '__storage_type' in 'Poco::Data::Statement'
   typedef typename _C::__storage_type    __storage_type;
           ~~~~~~~~~~~~~^~~~~~~~~~~~~~

Some day some std-defined type X will get defined that requires some typedef (value_type, difference_type, iterator_category, whatever), that also defines a template <classT> swap(X<T>&, X<T>&), and it will break all code that calls std::swap<MyType>(...).

The cure is two-fold:

1.  Don't specify the template argument when you call swap.  Just call swap(x, y).

2.  Don't qualify namespace std when you call swap unless you *know* that you need to call a swap in namespace std.  If you don't know, issue a using std::swap first, to let ADL work its magic.

The first rule is more important than the second.

Here is the libc++ diff that fixed this problem.  All that was done was to to create a fake __bit_reference<T> that did not require T to have any nested typedefs.

Index: include/bitset
===================================================================
--- include/bitset	(revision 134326)
+++ include/bitset	(revision 134327)
@@ -130,6 +130,15 @@
_LIBCPP_BEGIN_NAMESPACE_STD

template <size_t _N_words, size_t _Size>
+class __bitset;
+
+template <size_t _N_words, size_t _Size>
+struct __has_storage_type<__bitset<_N_words, _Size> >
+{
+    static const bool value = true;
+};
+
+template <size_t _N_words, size_t _Size>
class __bitset
{
public:
Index: include/vector
===================================================================
--- include/vector	(revision 134326)
+++ include/vector	(revision 134327)
@@ -1749,6 +1749,12 @@
template <class _Allocator> struct hash<vector<bool, _Allocator> >;

template <class _Allocator>
+struct __has_storage_type<vector<bool, _Allocator> >
+{
+    static const bool value = true;
+};
+
+template <class _Allocator>
class _LIBCPP_VISIBLE vector<bool, _Allocator>
    : private __vector_base_common<true>
{
@@ -1757,8 +1763,6 @@
    typedef bool                                     value_type;
    typedef _Allocator                               allocator_type;
    typedef allocator_traits<allocator_type>         __alloc_traits;
-    typedef __bit_reference<vector>                  reference;
-    typedef __bit_const_reference<vector>            const_reference;
    typedef typename __alloc_traits::size_type       size_type;
    typedef typename __alloc_traits::difference_type difference_type;
    typedef __bit_iterator<vector, false>            pointer;
@@ -1798,6 +1802,9 @@
    size_type                                              __size_;
    __compressed_pair<size_type, __storage_allocator> __cap_alloc_;

+    typedef __bit_reference<vector>                  reference;
+    typedef __bit_const_reference<vector>            const_reference;
+
    _LIBCPP_INLINE_VISIBILITY
    size_type& __cap() _NOEXCEPT
        {return __cap_alloc_.first();}
Index: include/__bit_reference
===================================================================
--- include/__bit_reference	(revision 134326)
+++ include/__bit_reference	(revision 134327)
@@ -21,7 +21,13 @@
template <class _C, bool _IsConst> class __bit_iterator;
template <class _C> class __bit_const_reference;

-template <class _C>
+template <class _Tp>
+struct __has_storage_type
+{
+    static const bool value = false;
+};
+
+template <class _C, bool = __has_storage_type<_C>::value>
class __bit_reference
{
    typedef typename _C::__storage_type    __storage_type;
@@ -66,6 +72,11 @@
        : __seg_(__s), __mask_(__m) {}
};

+template <class _C>
+class __bit_reference<_C, false>
+{
+};
+
template <class _C, class _D>
_LIBCPP_INLINE_VISIBILITY inline
void

Howard