interval_map: non_const iterator

The interval_map code cannot cope with iterators which change the size
of an interval. Due to this, they use const iterators. However, many
other modifications to intervals ARE ok and more efficient, nicer
looking code can be written with them.

This PR adds non-const iterators, but also adds some policing that the
size of the bufferlist has not changed over the interval.

Everything is hidden behind a template, as this changes the behaviour of interval map in a way that we don't want to use without careful testing of each instance.

Signed-off-by: Alex Ainscow <aainscow@uk.ibm.com>
(cherry picked from commit 360b079bc5e703754a7c27091370f910a78de2ba)

diff --git a/src/common/interval_map.h b/src/common/interval_map.h
index 9ef7fcbe1549d9708f0a0f0b46382988e8f379d4..8817246a20c73331d7b3ded17e35111e29aebc6b 100644 (file)
--- a/src/common/interval_map.h
+++ b/src/common/interval_map.h
@@ -30,7 +30,7 @@
  * commutativity, which doesn't work if we want more recent insertions
  * to overwrite previous ones.
  */
-template <typename K, typename V, typename S, template<typename, typename, typename ...> class C = std::map>
+template <typename K, typename V, typename S, template<typename, typename, typename ...> class C = std::map, bool nonconst_iterator = false>
 class interval_map {
   S s;
   using Map = C<K, std::pair<K, V> >;
@@ -215,6 +215,7 @@ public:
       set.insert(i.get_off(), i.get_len());
     }
   }
+
   class const_iterator {
     Cmapiter it;
     const_iterator(Cmapiter &&it) : it(std::move(it)) {}
@@ -269,6 +270,77 @@ public:
   const_iterator end() const {
     return const_iterator(m.end());
   }
+
+  const_iterator cbegin() const {
+    return const_iterator(m.begin());
+  }
+  const_iterator cend() const {
+    return const_iterator(m.end());
+  }
+
+  class iterator {
+    Mapiter it;
+    bool end;
+    iterator(Mapiter &&it, bool end) : it(std::move(it)), end(end) {}
+
+    friend class interval_map;
+  public:
+    iterator(const iterator &) = default;
+    iterator &operator=(const iterator &) = default;
+
+    iterator &operator++() {
+      /* While the buffer can be modified with a non-const iterator, it
+       * not change size. Allow changes in size would allow for the interval
+       * to be merged into the next one, which would allow for unexpected
+       * behaviour.
+       */
+      if (!end && get_val().length() != get_len()) {
+        throw std::out_of_range("buffer length has changed");
+      }
+      ++it;
+      return *this;
+    }
+    iterator operator++(int) {
+      return const_iterator(it++);
+    }
+    iterator &operator--() {
+      --it;
+      return *this;
+    }
+    iterator operator--(int) {
+      return const_iterator(it--);
+    }
+    bool operator==(const iterator &rhs) const {
+      return it == rhs.it;
+    }
+    bool operator!=(const iterator &rhs) const {
+      return it != rhs.it;
+    }
+    K get_off() const {
+      return it->first;
+    }
+    K get_len() const {
+      return it->second.first;
+    }
+    V &get_val() {
+      return it->second.second;
+    }
+    iterator &operator*() {
+      return *this;
+    }
+    constexpr bool contains(K _off, K _len) const {
+      K off = get_off();
+      K len = get_len();
+      return off <= _off && _off + _len <= off + len;
+    }
+  };
+  static constexpr bool nonconst_iterator_cond() { return nonconst_iterator; }
+  iterator begin() requires (nonconst_iterator) {
+    return iterator(m.begin(), false);
+  }
+  iterator end() requires (nonconst_iterator) {
+    return iterator(m.end(), true);
+  }
   std::pair<const_iterator, const_iterator> get_containing_range(
     K off,
     K len) const {

diff --git a/src/test/common/test_interval_map.cc b/src/test/common/test_interval_map.cc
index 04c3379ddbb4e5b9e8b77cee14ee9c6b83380a58..e2f96bf55ea2c8a471670e489cbeeffd4279d68e 100644 (file)
--- a/src/test/common/test_interval_map.cc
+++ b/src/test/common/test_interval_map.cc
@@ -27,7 +27,7 @@ public:
   using TestType = T;
 };
 
-template <typename _key, typename _can_merge, template<typename, typename, typename ...> class _map = std::map>
+template <typename _key, typename _can_merge, template<typename, typename, typename ...> class _map = std::map, bool _nonconst_iterator = false>
 struct bufferlist_test_type {
   using key = _key;
   using value = bufferlist;
@@ -62,7 +62,7 @@ struct bufferlist_test_type {
   };
 
   using splitter = boost::mpl::apply<make_splitter, _can_merge>;
-  using imap = interval_map<key, value, splitter, _map>;
+  using imap = interval_map<key, value, splitter, _map, _nonconst_iterator>;
 
   struct generate_random {
     bufferlist operator()(key len) {
@@ -81,7 +81,8 @@ using IntervalMapTypes = ::testing::Types<
   bufferlist_test_type<uint64_t, std::false_type>,
   bufferlist_test_type<uint64_t, std::true_type>,
   bufferlist_test_type<uint64_t, std::false_type, boost::container::flat_map>,
-  bufferlist_test_type<uint64_t, std::true_type, boost::container::flat_map>
+  bufferlist_test_type<uint64_t, std::true_type, boost::container::flat_map>,
+  bufferlist_test_type<uint64_t, std::true_type, boost::container::flat_map, true>
 >;
 
 TYPED_TEST_SUITE(IntervalMapTest, IntervalMapTypes);
@@ -405,3 +406,27 @@ TYPED_TEST(IntervalMapTest, print) {
     EXPECT_EQ("{0~5(5),10~5(5)}", out.str() );
   }
 }
+
+/* This test does nothing unless nonconst_iterator is set on the interval map
+ * If it is set, then the simple fact that append_zero() compiles means that
+ * the non-const iterator has been enabled and used. The test then checks that
+ * changing the size of a buffer is illegal.
+ */
+TYPED_TEST(IntervalMapTest, append_buffer_in_loop) {
+  USING_WITH_MERGE;
+  imap m;
+  if constexpr (m.nonconst_iterator_cond()) {
+    m.insert(0, 5, gen(5));
+    try {
+      for (auto && i : m) {
+        i.get_val().append_zero(10);
+      }
+      FAIL(); // Should panic
+    } catch (const std::out_of_range& exception) {
+      ASSERT_EQ("buffer length has changed"sv, exception.what());
+    } catch (const std::exception&) {
+      // Wrong exception.
+      FAIL();
+    }
+  }
+}
\ No newline at end of file