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 <[email protected]>

Author: aainscow

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 @@ class interval_map {
       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 @@ class interval_map {
   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 {

src/test/common/test_interval_map.cc

@@ -27,7 +27,7 @@ class IntervalMapTest : public ::testing::Test {
   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();
+    }
+  }
+}