[libc++] Consolidate swap, swap_ranges, and iter_swap in <type_traits>. NFC.

[• Quuxplusone]

Summary

Right now in master, <type_traits> contains the implementation of swap(T&, T&), a forward-declaration of swap(T(&)[N], T(&)[N]), and the implementation of iter_swap in terms of ADL swap. Then, <utility> (which includes <type_traits>) contains the implementation of swap(T(&)[N], T(&)[N]) in terms of std::swap_ranges, plus the implementation of swap_ranges.

This means that when the user's program does #include <type_traits>, they get a declaration but no definition for swap(T(&)[N], T(&)[N]) — not technically wrong, not a bug, but mildly surprising. Also, this seems to mean that changing the enable_if business on swap(T(&)[N], T(&)[N]) would count as an ABI break (if only for user programs that fail to include-what-they-use and thus technically are non-conforming IIUC).

Most importantly, it means that when I want to modify swap (say, to add another overload specifically for trivially relocatable types), there are two headers to modify instead of just one. :)

After this patch, <type_traits> contains the implementation of swap(T&, T&); the implementation of swap(T(&)[N], T(&)[N]) in terms of std::swap_ranges; the implementation of swap_ranges; and the implementation of iter_swap in terms of ADL swap. It's a one-stop swap shop! Then <utility> simply includes <type_traits>, as before.

I don't intend this patch to have any observable effect for users.

[ldionne]

I don't mind this change.

However, I really dislike the general organization (or rather, the lack thereof) in libc++. It's weird to be defining swap and swap_ranges inside <type_traits>, when swap_ranges should be part of <algorithm> and swap should be in <utility>.

[• Quuxplusone]

I really dislike the general organization (or rather, the lack thereof) in libc++. It's weird to be defining swap and swap_ranges inside <type_traits>, when swap_ranges should be part of <algorithm> and swap should be in <utility>.

I'd call that a lack of organization in the paper standard, not in the implementation! According to N4810,

• <concepts> defines Swappable in terms of ranges::swap, and defines ranges::swap in terms of ADL swap
• <utility> defines swap(T&, T&), and defines swap(T (&)[N], T (&)[N])
• <type_traits> defines is_swappable in terms of std::swap and ADL swap
• <iterator> defines ranges::iter_swap in terms of ranges::swap and ADL iter_swap(!!)
• <algorithm> defines std::iter_swap in terms of std::swap and (arguably) ADL swap; defines std::swap_ranges in terms of std::swap and (arguably) ADL swap; defines ranges::swap_ranges in terms of ranges::iter_swap

• <concepts> includes nothing
• <utility> includes only <initializer_list>
• <type_traits> includes nothing, despite requiring std::swap out of <utility>
• <iterator> includes only <concepts>
• <algorithm> includes only <initializer_list>, despite requiring std::swap out of <utility> and ranges::iter_swap out of <iterator>

Taking that spaghetti literally would result in a maintenance nightmare.

[• Quuxplusone]

I guess this has been languishing. @ldionne, thoughts on maybe landing this patch?

[• Quuxplusone]

Rebased on master; the patch still applied cleanly. @ldionne ping?

[mclow.lists]

zoecarver accepted this revision

@zoecarver is not an approver for libc++.

[zoecarver]

Sorry, I simply meant "this looks good to me." Next time, I will just comment that :)

[• Quuxplusone]

I assume @ldionne _is_ an approver for libc++ at this point? And either way, I haven't got commit access, which is why I continue to ping Louis despite his earlier marking this patch as accepted.

[mclow.lists]

With the comment change I suggested, I am fine with this.

include/utility

253

I would just say:

// swap_ranges is defined in <type_traits>`

// swap is defined in <type_traits>

and leave it at that.

[zoecarver]

Thanks for the patch @Quuxplusone. Committed as rL371639.

[ldionne]

Sorry -- yes, I am fine with this patch. I just forgot to commit it.

include/type_traits

 
 #endif // _LIBCPP_STD_VER > 14
 
 #endif  // !defined(_LIBCPP_CXX03_LANG)
 
 template <class _Tp> struct __is_swappable;
 template <class _Tp> struct __is_nothrow_swappable;
 
+// swap, swap_ranges
+
+template <class _ForwardIterator1, class _ForwardIterator2>
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
+_ForwardIterator2
+swap_ranges(_ForwardIterator1 __first1, _ForwardIterator1 __last1, _ForwardIterator2 __first2);
+
 template <class _Tp>
 inline _LIBCPP_INLINE_VISIBILITY
 #ifndef _LIBCPP_CXX03_LANG
 typename enable_if
 <
     is_move_constructible<_Tp>::value &&
     is_move_assignable<_Tp>::value
 >::type

     __y = _VSTD::move(__t);
 }
 
 template<class _Tp, size_t _Np>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
 typename enable_if<
     __is_swappable<_Tp>::value
 >::type
-swap(_Tp (&__a)[_Np], _Tp (&__b)[_Np]) _NOEXCEPT_(__is_nothrow_swappable<_Tp>::value);
+swap(_Tp (&__a)[_Np], _Tp (&__b)[_Np]) _NOEXCEPT_(__is_nothrow_swappable<_Tp>::value)
+{
+    _VSTD::swap_ranges(__a, __a + _Np, __b);
+}
+
+template <class _ForwardIterator1, class _ForwardIterator2>
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
+_ForwardIterator2
+swap_ranges(_ForwardIterator1 __first1, _ForwardIterator1 __last1, _ForwardIterator2 __first2)
+{
+    for(; __first1 != __last1; ++__first1, (void) ++__first2)
+        swap(*__first1, *__first2);
+    return __first2;
+}
+
+// iter_swap
 
 template <class _ForwardIterator1, class _ForwardIterator2>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
 void
 iter_swap(_ForwardIterator1 __a, _ForwardIterator2 __b)
     //                                  _NOEXCEPT_(_NOEXCEPT_(swap(*__a, *__b)))
                _NOEXCEPT_(_NOEXCEPT_(swap(*_VSTD::declval<_ForwardIterator1>(),
                                           *_VSTD::declval<_ForwardIterator2>())))

include/utility

 {
     return !(__x < __y);
 }
 
 }  // rel_ops
 
 // swap_ranges
 
+// moved to <type_traits> for better swap / noexcept support
 
-template <class _ForwardIterator1, class _ForwardIterator2>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
-_ForwardIterator2
-swap_ranges(_ForwardIterator1 __first1, _ForwardIterator1 __last1, _ForwardIterator2 __first2)
-{
-    for(; __first1 != __last1; ++__first1, (void) ++__first2)
-        swap(*__first1, *__first2);
-    return __first2;
-}
+// swap
 
-// forward declared in <type_traits>
-template<class _Tp, size_t _Np>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
-typename enable_if<
-    __is_swappable<_Tp>::value
->::type
-swap(_Tp (&__a)[_Np], _Tp (&__b)[_Np]) _NOEXCEPT_(__is_nothrow_swappable<_Tp>::value)
-{
-    _VSTD::swap_ranges(__a, __a + _Np, __b);
-}
+// moved to <type_traits> for better swap / noexcept support
+
+// move_if_noexcept
 
 template <class _Tp>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
 #ifndef _LIBCPP_CXX03_LANG
 typename conditional
 <
     !is_nothrow_move_constructible<_Tp>::value && is_copy_constructible<_Tp>::value,
     const _Tp&,