Reapply reverted iterator invalidation fix and improve buffers test performance

Author: MungoG

include/boost/buffers/slice.hpp

@@ -64,9 +64,12 @@ class slice_of
     using iter_type = decltype(
         std::declval<BufferSequence const&>().begin());
 
+    using difference_type =
+        typename std::iterator_traits<iter_type>::difference_type;
+
     BufferSequence bs_;
-    iter_type begin_;
-    iter_type end_;
+    difference_type begin_ = 0; // index of first buffer in sequence
+    difference_type end_ = 0;   // 1 + index of last buffer in sequence
     std::size_t len_ = 0;       // length of bs_
     std::size_t size_ = 0;      // total bytes
     std::size_t prefix_ = 0;    // used prefix bytes
@@ -92,11 +95,12 @@ class slice_of
     slice_of(
         BufferSequence const& bs)
         : bs_(bs)
-        , begin_(buffers::begin(bs_))
-        , end_(buffers::end(bs_))
     {
-        auto it = begin_;
-        while(it != end_)
+        iter_type it = buffers::begin(bs_);
+        iter_type eit = buffers::end(bs_);
+        begin_ = 0;
+        end_ = std::distance(it, eit);
+        while(it != eit)
         {
             value_type b(*it);
             size_ += b.size();
@@ -127,18 +131,39 @@ class slice_of
     }
 
 private:
+    iter_type
+    begin_iter_impl() const noexcept
+    {
+        iter_type it = buffers::begin(bs_);
+        std::advance(it, begin_);
+        return it;
+    }
+
+    iter_type
+    end_iter_impl() const noexcept
+    {
+        iter_type it = buffers::begin(bs_);
+        std::advance(it, end_);
+        return it;
+    }
+
     void
     remove_prefix_impl(
         std::size_t n)
     {
+        if(n > size_)
+            n = size_;
+
         // nice hack to simplify the loop (M. Nejati)
         n += prefix_;
         size_ += prefix_;
         prefix_ = 0;
 
+        iter_type it = begin_iter_impl();
+
         while(n > 0 && begin_ != end_)
         {
-            value_type b = *begin_;
+            value_type b = *it;
             if(n < b.size())
             {
                 prefix_ = n;
@@ -148,6 +173,7 @@ class slice_of
             n -= b.size();
             size_ -= b.size();
             ++begin_;
+            ++it;
             --len_;
         }
     }
@@ -162,12 +188,19 @@ class slice_of
             return;
         }
         BOOST_ASSERT(begin_ != end_);
+
+        if(n > size_)
+            n = size_;
+
         n += suffix_;
         size_ += suffix_;
         suffix_ = 0;
-        iter_type it = end_;
-        --it;
-        while(it != begin_)
+
+        iter_type bit = begin_iter_impl();
+        iter_type it = end_iter_impl();
+        it--;
+
+        while(it != bit)
         {
             value_type b = *it;
             if(n < b.size())
@@ -192,6 +225,7 @@ class slice_of
         }
         end_ = begin_;
         len_ = 0;
+        size_ = 0;
     }
 
     void
@@ -376,7 +410,7 @@ begin() const noexcept ->
     const_iterator
 {
     return const_iterator(
-        this->begin_, prefix_, suffix_, 0, len_);
+        begin_iter_impl(), prefix_, suffix_, 0, len_);
 }
 
 template<class BufferSequence>
@@ -386,7 +420,7 @@ end() const noexcept ->
     const_iterator
 {
     return const_iterator(
-        this->end_, prefix_, suffix_, len_, len_);
+        end_iter_impl(), prefix_, suffix_, len_, len_);
 }
 
 //------------------------------------------------

test/unit/CMakeLists.txt

@@ -7,7 +7,9 @@
 # Official repository: https://github.com/cppalliance/buffers
 #
 
-add_subdirectory(../../../url/extra/test_suite test_suite)
+if (NOT TARGET boost_url_test_suite)
+    add_subdirectory(../../../url/extra/test_suite test_suite)
+endif()
 
 file(GLOB_RECURSE PFILES CONFIGURE_DEPENDS *.cpp *.hpp)
 list(APPEND PFILES

test/unit/slice.cpp

@@ -17,6 +17,7 @@
 #include <boost/static_assert.hpp>
 
 #include <array>
+#include <vector>
 
 #include "test_buffers.hpp"
 #include "test_suite.hpp"
@@ -116,10 +117,13 @@ struct slice_test
             return;
         if(! BOOST_TEST_EQ(core::string_view(buf, n), s))
             return;
-        test::check_iterators(b, s);
+
+        std::string tmp;
+        test::check_iterators(b, s, tmp);
     }
 
-    using seq_type = std::array<const_buffer, 3>;
+    // Use a vector so that iterator invalidation is observable during testing.
+    using seq_type = std::vector<const_buffer>;
 
     void
     grind_back(
@@ -176,9 +180,9 @@ struct slice_test
     run()
     {
         std::string s;
-        seq_type bs =  make_buffers(s,
-            "boost.", "buffers.", "slice_" );
-        test::check_sequence(bs, s);
+        auto a = make_buffers(s, "boost.", "buffers.", "slice_");
+        seq_type bs(a.begin(), a.end());
+        test::check_sequence(bs, s, true);
         //check(bs, s);
         //grind(bs, s);
     }

test/unit/test_buffers.hpp

@@ -99,16 +99,21 @@ template<class ConstBufferSequence>
 void
 check_iterators(
     ConstBufferSequence bs,
-    core::string_view pat)
+    core::string_view pat,
+    std::string& s)
 {
     BOOST_ASSERT(is_const_buffer_sequence<ConstBufferSequence>::value);
     BOOST_TEST_EQ(size(bs), pat.size());
 
     auto const& ct = bs;
 
+    //std::string s;
+    s.reserve(pat.size() + 1);
+
     // operator++()
     {
-        std::string s;
+        s.clear();
+        s.reserve(pat.size() + 1);
         auto it = begin(bs);
         auto const end_ = end(bs);
         while(it != end_)
@@ -124,7 +129,8 @@ check_iterators(
 
     // operator++(int)
     {
-        std::string s;
+        s.clear();
+        s.reserve(pat.size() + 1);
         auto it = begin(bs);
         auto const end_ = end(bs);
         while(it != end_)
@@ -140,7 +146,8 @@ check_iterators(
 
     // operator++() const
     {
-        std::string s;
+        s.clear();
+        s.reserve(pat.size() + 1);
         auto it = begin(ct);
         auto const end_ = end(ct);
         while(it != end_)
@@ -156,7 +163,8 @@ check_iterators(
 
     // operator++(int) const
     {
-        std::string s;
+        s.clear();
+        s.reserve(pat.size() + 1);
         auto it = begin(ct);
         auto const end_ = end(ct);
         while(it != end_)
@@ -172,7 +180,8 @@ check_iterators(
 
     // operator--()
     {
-        std::string s;
+        s.clear();
+        s.reserve(pat.size() + 1);
         auto it = end(bs);
         auto const begin_ = begin(bs);
         while(it != begin_)
@@ -188,7 +197,8 @@ check_iterators(
 
     // operator--(int)
     {
-        std::string s;
+        s.clear();
+        s.reserve(pat.size() + 1);
         auto it = end(bs);
         auto const begin_ = begin(bs);
         while(it != begin_)
@@ -204,7 +214,8 @@ check_iterators(
 
     // operator--() const
     {
-        std::string s;
+        s.clear();
+        s.reserve(pat.size() + 1);
         auto it = end(ct);
         auto const begin_ = begin(ct);
         while(it != begin_)
@@ -220,7 +231,8 @@ check_iterators(
 
     // operator--(int) const
     {
-        std::string s;
+        s.clear();
+        s.reserve(pat.size() + 1);
         auto it = end(ct);
         auto const begin_ = begin(ct);
         while(it != begin_)
@@ -255,23 +267,44 @@ template<class ConstBufferSequence>
 void
 grind_front(
     ConstBufferSequence const& bs0,
-    core::string_view pat0)
+    core::string_view pat0,
+    bool deep)
 {
+    std::string tmp;
+
     for(std::size_t n = 0; n <= pat0.size() + 1; ++n)
     {
         {
             auto pat = trimmed_front(pat0, n);
             slice_type<ConstBufferSequence> bs(bs0);
             remove_prefix(bs, n);
             check_eq(bs, pat);
-            check_iterators(bs, pat);
+            check_iterators(bs, pat, tmp);
+
+            if(deep)
+            {
+                // Take a copy, blank out the original to invalidate any
+                // iterators, and redo the test
+                slice_type<ConstBufferSequence> bsc(bs);
+                {
+                    slice_type<ConstBufferSequence> dummy{};
+                    std::swap(bs, dummy);
+                }
+                for(std::size_t m = 0; m <= pat.size() + 1; ++m)
+                {
+                    auto pat2 = trimmed_front(pat, m);
+                    slice_type<ConstBufferSequence> bs2(bsc);
+                    remove_prefix(bs2, m);
+                    check_eq(bs2, pat2);
+                }
+            }
         }
         {
             auto pat = kept_front(pat0, n);
             slice_type<ConstBufferSequence> bs(bs0);
             keep_prefix(bs, n);
             check_eq(bs, pat);
-            check_iterators(bs, pat);
+            check_iterators(bs, pat, tmp);
         }
     }
 }
@@ -280,23 +313,43 @@ template<class ConstBufferSequence>
 void
 grind_back(
     ConstBufferSequence const& bs0,
-    core::string_view pat0)
+    core::string_view pat0,
+    bool deep)
 {
+    std::string tmp;
+
     for(std::size_t n = 0; n <= pat0.size() + 1; ++n)
     {
         {
             auto pat = trimmed_back(pat0, n);
             slice_type<ConstBufferSequence> bs(bs0);
             remove_suffix(bs, n);
             check_eq(bs, pat);
-            check_iterators(bs, pat);
+            check_iterators(bs, pat, tmp);
+            if(deep)
+            {
+                // Take a copy, blank out the original to invalidate any
+                // iterators, and redo the test
+                slice_type<ConstBufferSequence> bsc(bs);
+                {
+                    slice_type<ConstBufferSequence> dummy{};
+                    std::swap(bs, dummy);
+                }
+                for(std::size_t m = 0; m <= pat.size() + 1; ++m)
+                {
+                    auto pat2 = trimmed_back(pat, m);
+                    slice_type<ConstBufferSequence> bs2(bsc);
+                    remove_suffix(bs2, m);
+                    check_eq(bs2, pat2);
+                }
+            }
         }
         {
             auto pat = kept_back(pat0, n);
             slice_type<ConstBufferSequence> bs(bs0);
             keep_suffix(bs, n);
             check_eq(bs, pat);
-            check_iterators(bs, pat);
+            check_iterators(bs, pat, tmp);
         }
     }
 }
@@ -305,22 +358,24 @@ template<class ConstBufferSequence>
 void
 check_slice(
     ConstBufferSequence const& bs,
-    core::string_view pat)
+    core::string_view pat,
+    bool deep)
 {
-    grind_front(bs, pat);
-    grind_back(bs, pat);
+    grind_front(bs, pat, deep);
+    grind_back(bs, pat, deep);
 }
 
 // Test API and behavior of a BufferSequence
 template<class T>
 void
 check_sequence(
-    T const& t, core::string_view pat)
+    T const& t, core::string_view pat, bool deep = false)
 {
     BOOST_STATIC_ASSERT(is_const_buffer_sequence<T>::value);
 
-    check_iterators(t, pat);
-    check_slice(t, pat);
+    std::string tmp;
+    check_iterators(t, pat, tmp);
+    check_slice(t, pat, deep);
 }
 
 } // test