[cfe-dev] Specializing std::swap

[Aleksandar Fabijanic]

Thanks, Howard - that was the explanation I was looking for.

Alex

On Wed, May 22, 2013 at 10:35 PM, Howard Hinnant <hhinnant at apple.com> wrote:
> 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
>