Fix thinko in ranges::iter_swap (#1072)

...by transposing arguments to `_Iter_exchange_move`. Fixes #1067
microsoft · Jul 30, 2020 · 30777d5 · 30777d5
1 parent e9f56a6
commit 30777d5
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 11 deletions.
diff --git a/stl/inc/xutility b/stl/inc/xutility
@@ -1038,10 +1038,10 @@ namespace ranges {
         // clang-format on
 
         template <class _Xty, class _Yty>
-        _NODISCARD constexpr iter_value_t<remove_reference_t<_Xty>> _Iter_exchange_move(
-            _Xty&& _XVal, _Yty&& _YVal) noexcept(noexcept(iter_value_t<remove_reference_t<_Xty>>(iter_move(_XVal)))) {
-            iter_value_t<remove_reference_t<_Xty>> _Tmp(iter_move(_XVal));
-            *_XVal = iter_move(_YVal);
+        _NODISCARD constexpr iter_value_t<remove_reference_t<_Xty>> _Iter_exchange_move(_Xty&& _XVal,
+            _Yty&& _YVal) noexcept(noexcept(iter_value_t<remove_reference_t<_Xty>>(_RANGES iter_move(_XVal)))) {
+            iter_value_t<remove_reference_t<_Xty>> _Tmp(_RANGES iter_move(_XVal));
+            *_XVal = _RANGES iter_move(_YVal);
             return _Tmp;
         }
 
@@ -1057,7 +1057,7 @@ namespace ranges {
                     return {_St::_Swap, noexcept(_RANGES swap(*_STD declval<_Ty1>(), *_STD declval<_Ty2>()))};
                 } else if constexpr (_Symmetric_indirectly_movable_storable<_Ty1, _Ty2>) {
                     constexpr auto _Nothrow = noexcept(
-                        *_STD declval<_Ty1>() = _Iter_exchange_move(_STD declval<_Ty1>(), _STD declval<_Ty2>()));
+                        *_STD declval<_Ty1>() = _Iter_exchange_move(_STD declval<_Ty2>(), _STD declval<_Ty1>()));
                     return {_St::_Exchange, _Nothrow};
                 } else {
                     return {_St::_None};
@@ -1078,7 +1078,7 @@ namespace ranges {
                     _RANGES swap(*static_cast<_Ty1&&>(_Val1), *static_cast<_Ty2&&>(_Val2));
                 } else if constexpr (_Choice<_Ty1, _Ty2>._Strategy == _St::_Exchange) {
                     *static_cast<_Ty1&&>(_Val1) =
-                        _Iter_exchange_move(static_cast<_Ty1&&>(_Val1), static_cast<_Ty2&&>(_Val2));
+                        _Iter_exchange_move(static_cast<_Ty2&&>(_Val2), static_cast<_Ty1&&>(_Val1));
                 } else {
                     static_assert(_Always_false<_Ty1>, "should be unreachable");
                 }

diff --git a/tests/std/tests/P0896R4_ranges_iterator_machinery/test.cpp b/tests/std/tests/P0896R4_ranges_iterator_machinery/test.cpp
@@ -1267,11 +1267,28 @@ namespace iterator_cust_swap_test {
         // clang-format on
 
         constexpr bool test() {
-            int i0 = 42, i1 = 13;
-            S s0{i0}, s1{i1};
-            ranges::iter_swap(s0, s1);
-            assert(i0 == 13);
-            assert(i1 == 42);
+            {
+                int i0 = 42;
+                int i1 = 13;
+                S s0{i0};
+                S s1{i1};
+                ranges::iter_swap(s0, s1);
+                assert(i0 == 13);
+                assert(i1 == 42);
+            }
+
+#if !defined(__clang__) && !defined(__EDG__) // TRANSITION, VSO-938163
+            if (!std::is_constant_evaluated())
+#endif // TRANSITION, VSO-938163
+            {
+                // Validate iter_swap bullet 3 to defend against regression of GH-1067 "ranges::iter_swap is broken"
+                int i  = 42;
+                long l = 13;
+                ranges::iter_swap(&i, &l);
+                assert(i == 13);
+                assert(l == 42);
+            }
+
             return true;
         }
         STATIC_ASSERT(test());