<ranges>: Invalidate the cached position of a view after move (#1931)

Co-authored-by: Casey Carter <[email protected]> Co-authored-by: Stephan T. Lavavej <[email protected]>
microsoft · Aug 6, 2021 · b03f22f · b03f22f
1 parent 5d94b31
commit b03f22f
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 4 deletions.
diff --git a/stl/inc/ranges b/stl/inc/ranges
@@ -187,13 +187,26 @@ namespace ranges {
 
         // a copied iterator doesn't point into a copied range, so cache values must not propagate via copy
         constexpr _Cached_position(const _Cached_position&) noexcept(is_nothrow_default_constructible_v<_It>) {}
-        constexpr _Cached_position& operator=(const _Cached_position&) noexcept(
-            is_nothrow_default_constructible_v<_It>) {
+        constexpr _Cached_position& operator=(const _Cached_position&) noexcept(noexcept(_Pos = _It{})) {
             _Pos    = _It{};
             _Cached = false;
             return *this;
         }
 
+        // a moved iterator doesn't point into a moved range, so cache values must not propagate via move;
+        // similarly, a cache value might not be valid for a moved-from view so clear move sources
+        constexpr _Cached_position(_Cached_position&& _Other) noexcept(noexcept(_Pos = _It{})) {
+            _Other._Pos    = _It{};
+            _Other._Cached = false;
+        }
+        constexpr _Cached_position& operator=(_Cached_position&& _Other) noexcept(noexcept(_Pos = _It{})) {
+            _Pos           = _It{};
+            _Cached        = false;
+            _Other._Pos    = _It{};
+            _Other._Cached = false;
+            return *this;
+        }
+
         _NODISCARD constexpr bool _Has_cache() const noexcept { // Is there a cached position?
             return _Cached;
         }
@@ -203,7 +216,7 @@ namespace ranges {
             return _Pos;
         }
 
-        _NODISCARD constexpr void _Set_cache(_Rng&, _It _Iter) noexcept(is_nothrow_move_assignable_v<_It>) {
+        constexpr void _Set_cache(_Rng&, _It _Iter) noexcept(is_nothrow_move_assignable_v<_It>) {
             _Pos    = _STD move(_Iter);
             _Cached = true;
         }
@@ -224,6 +237,14 @@ namespace ranges {
         _Cached_position(const _Cached_position&) = default;
         _Cached_position& operator=(const _Cached_position&) = default;
 
+        // Offsets are potentially invalidated by move, so source caches are invalidated after move
+        constexpr _Cached_position(_Cached_position&& _Other) noexcept
+            : _Off(_STD exchange(_Other._Off, range_difference_t<_Rng>{-1})) {}
+        constexpr _Cached_position& operator=(_Cached_position&& _Other) noexcept {
+            _Off = _STD exchange(_Other._Off, range_difference_t<_Rng>{-1});
+            return *this;
+        }
+
         _NODISCARD constexpr bool _Has_cache() const noexcept { // Is there a cached position?
             return _Off >= range_difference_t<_Rng>{0};
         }
@@ -233,7 +254,7 @@ namespace ranges {
             return _RANGES begin(_Range) + _Off;
         }
 
-        _NODISCARD constexpr void _Set_cache(_Rng& _Range, const _It& _Iter) noexcept(
+        constexpr void _Set_cache(_Rng& _Range, const _It& _Iter) noexcept(
             noexcept(_Off = _Iter - _RANGES begin(_Range))) {
             _Off = _Iter - _RANGES begin(_Range);
         }

diff --git a/tests/std/test.lst b/tests/std/test.lst
@@ -176,6 +176,7 @@ tests\GH_001530_binomial_accuracy
 tests\GH_001541_case_sensitive_boolalpha
 tests\GH_001638_dllexport_derived_classes
 tests\GH_001850_clog_tied_to_cout
+tests\GH_001914_cached_position
 tests\GH_002039_byte_is_not_trivially_swappable
 tests\GH_002058_debug_iterator_race
 tests\LWG2597_complex_branch_cut

diff --git a/tests/std/tests/GH_001914_cached_position/env.lst b/tests/std/tests/GH_001914_cached_position/env.lst
@@ -0,0 +1,4 @@
+# Copyright (c) Microsoft Corporation.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+RUNALL_INCLUDE ..\concepts_latest_matrix.lst
diff --git a/tests/std/tests/GH_001914_cached_position/test.cpp b/tests/std/tests/GH_001914_cached_position/test.cpp
@@ -0,0 +1,62 @@
+// Copyright (c) Microsoft Corporation.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include <cassert>
+#include <ranges>
+#include <utility>
+
+#include <range_algorithm_support.hpp>
+using namespace std;
+
+template <class View>
+class test_cache : public ranges::_Cached_position<View, test_cache<View>> {
+public:
+    constexpr test_cache(View view) : _view(move(view)) {}
+
+    constexpr void set_cache() {
+        this->_Set_cache(_view, _view.begin());
+    }
+
+    [[nodiscard]] constexpr bool has_cache() const noexcept {
+        return this->_Has_cache();
+    }
+
+private:
+    View _view;
+};
+
+template <ranges::forward_range R>
+constexpr bool test_one() {
+    constexpr int some_ints[] = {0, 1, 2, 3, 4, 5, 6, 7};
+    { // Propagate cache of random access range after copy
+        test_cache cache{R{some_ints}};
+        cache.set_cache();
+
+        test_cache new_cache = cache;
+        assert(cache.has_cache());
+        assert(new_cache.has_cache() == ranges::random_access_range<R>);
+    }
+
+    { // Propagate cache of random access range after move
+        test_cache cache{R{some_ints}};
+        cache.set_cache();
+
+        test_cache new_cache = move(cache);
+        assert(!cache.has_cache());
+        assert(new_cache.has_cache() == ranges::random_access_range<R>);
+    }
+    return true;
+}
+
+template <class Category>
+using test_range = test::range<Category, const int, test::Sized::no,
+    test::CanDifference{derived_from<Category, random_access_iterator_tag>}, test::Common::yes, test::CanCompare::yes,
+    test::ProxyRef::no, test::CanView::yes, test::Copyability::copyable>;
+
+int main() {
+    test_one<test_range<forward_iterator_tag>>();
+    static_assert(test_one<test_range<forward_iterator_tag>>());
+
+    test_one<test_range<random_access_iterator_tag>>();
+    static_assert(test_one<test_range<random_access_iterator_tag>>());
+}