From 719fe66b77e1d09a99e4cf44722f52c29d134de9 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Fri, 14 Dec 2018 20:25:38 -0800 Subject: [PATCH] Fix and refactor buffers_cat: Elements of zero size are correctly skipped during iteration. * Tidy up tests * Increase code coverage * Remove move special members * Correct behavior for default constructed iterators --- CHANGELOG.md | 1 + include/boost/beast/core/buffers_cat.hpp | 6 - include/boost/beast/core/impl/buffers_cat.hpp | 250 ++++----- test/beast/core/buffers_cat.cpp | 473 ++++++++---------- 4 files changed, 350 insertions(+), 380 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4994b411..28f1d527 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ Version 200 * buffers_cat fixes and coverage * Refactor buffers_adaptor * Refactor buffers_range +* Fix and refactor buffers_cat API Changes: diff --git a/include/boost/beast/core/buffers_cat.hpp b/include/boost/beast/core/buffers_cat.hpp index b85fedae..4576b392 100644 --- a/include/boost/beast/core/buffers_cat.hpp +++ b/include/boost/beast/core/buffers_cat.hpp @@ -46,15 +46,9 @@ public: /// Copy Constructor buffers_cat_view(buffers_cat_view const&) = default; - /// Move Constructor - buffers_cat_view(buffers_cat_view&&) = default; - /// Copy Assignment buffers_cat_view& operator=(buffers_cat_view const&) = default; - /// Move Assignment - buffers_cat_view& operator=(buffers_cat_view&&) = default; - /** Constructor @param buffers The list of buffer sequences to concatenate. diff --git a/include/boost/beast/core/impl/buffers_cat.hpp b/include/boost/beast/core/impl/buffers_cat.hpp index 758da311..9e86d686 100644 --- a/include/boost/beast/core/impl/buffers_cat.hpp +++ b/include/boost/beast/core/impl/buffers_cat.hpp @@ -35,6 +35,7 @@ struct buffers_cat_view_iterator_base net::mutable_buffer operator*() const { + // Dereferencing a one-past-the-end iterator BOOST_THROW_EXCEPTION(std::logic_error{ "invalid iterator"}); } @@ -77,9 +78,7 @@ public: std::bidirectional_iterator_tag; const_iterator() = default; - const_iterator(const_iterator&& other) = default; const_iterator(const_iterator const& other) = default; - const_iterator& operator=(const_iterator&& other) = default; const_iterator& operator=(const_iterator const& other) = default; bool @@ -103,11 +102,9 @@ public: const_iterator operator++(int); - // deprecated const_iterator& operator--(); - // deprecated const_iterator operator--(int); @@ -115,53 +112,6 @@ private: const_iterator( detail::tuple const& bn, bool at_end); - template - void - next(C const&) - { - if(net::buffer_size( - detail::get(*bn_)) != 0) - { - it_.template emplace( - net::buffer_sequence_begin( - detail::get(*bn_))); - return; - } - next(C{}); - } - - void - next(C const&) - { - // end - auto constexpr I = sizeof...(Bn); - it_.template emplace(); - } - - template - void - prev(C const&) - { - if(net::buffer_size( - detail::get(*bn_)) != 0) - { - it_.template emplace( - net::buffer_sequence_end( - detail::get(*bn_))); - return; - } - prev(C{}); - } - - void - prev(C<0> const&) - { - auto constexpr I = 0; - it_.template emplace( - net::buffer_sequence_end( - detail::get(*bn_))); - } - struct dereference { const_iterator const& self; @@ -190,15 +140,107 @@ private: void operator()(mp11::mp_size_t<0>) { + // Incrementing a default-constructed iterator BOOST_THROW_EXCEPTION(std::logic_error{ "invalid iterator"}); } + template + void + operator()(mp11::mp_size_t) + { + ++self.it_.template get(); + next(mp11::mp_size_t{}); + } + + template + void + next(mp11::mp_size_t) + { + auto& it = self.it_.template get(); + for(;;) + { + if (it == net::buffer_sequence_end( + detail::get(*self.bn_))) + break; + if(net::const_buffer(*it).size() > 0) + return; + ++it; + } + self.it_.template emplace( + net::buffer_sequence_begin( + detail::get(*self.bn_))); + next(mp11::mp_size_t{}); + } + + void + operator()(mp11::mp_size_t) + { + auto constexpr I = sizeof...(Bn); + ++self.it_.template get(); + next(mp11::mp_size_t{}); + } + + void + next(mp11::mp_size_t) + { + auto constexpr I = sizeof...(Bn); + auto& it = self.it_.template get(); + for(;;) + { + if (it == net::buffer_sequence_end( + detail::get(*self.bn_))) + break; + if(net::const_buffer(*it).size() > 0) + return; + ++it; + } + // end + self.it_.template emplace(); + } + [[noreturn]] void - operator()(mp11::mp_size_t) + operator()(mp11::mp_size_t) { - (*this)(mp11::mp_size_t<0>{}); + // Incrementing a one-past-the-end iterator + BOOST_THROW_EXCEPTION(std::logic_error{ + "invalid iterator"}); + } + }; + + struct decrement + { + const_iterator& self; + + [[noreturn]] + void + operator()(mp11::mp_size_t<0>) + { + // Decrementing a default-constructed iterator + BOOST_THROW_EXCEPTION(std::logic_error{ + "invalid iterator"}); + } + + void + operator()(mp11::mp_size_t<1>) + { + auto constexpr I = 1; + + auto& it = self.it_.template get(); + for(;;) + { + if(it == net::buffer_sequence_begin( + detail::get(*self.bn_))) + { + // Decrementing an iterator to the beginning + BOOST_THROW_EXCEPTION(std::logic_error{ + "invalid iterator"}); + } + --it; + if(net::const_buffer(*it).size() > 0) + return; + } } template @@ -206,53 +248,31 @@ private: operator()(mp11::mp_size_t) { auto& it = self.it_.template get(); - if (++it == net::buffer_sequence_end( - detail::get(*self.bn_))) - self.next(C()); + for(;;) + { + if(it == net::buffer_sequence_begin( + detail::get(*self.bn_))) + break; + --it; + if(net::const_buffer(*it).size() > 0) + return; + } + self.it_.template emplace( + net::buffer_sequence_end( + detail::get(*self.bn_))); + (*this)(mp11::mp_size_t{}); + } + + void + operator()(mp11::mp_size_t) + { + auto constexpr I = sizeof...(Bn)+1; + self.it_.template emplace( + net::buffer_sequence_end( + detail::get(*self.bn_))); + (*this)(mp11::mp_size_t{}); } }; - - void - decrement(C const&) - { - auto constexpr I = sizeof...(Bn); - if(it_.index() == I+1) - prev(C{}); - decrement(C{}); - } - - template - void - decrement(C const&) - { - if(it_.index() == I+1) - { - if(it_.template get() != - net::buffer_sequence_begin( - detail::get(*bn_))) - { - --it_.template get(); - return; - } - prev(C{}); - } - decrement(C{}); - } - - void - decrement(C<0> const&) - { - auto constexpr I = 0; - if(it_.template get() != - net::buffer_sequence_begin( - detail::get(*bn_))) - { - --it_.template get(); - return; - } - BOOST_THROW_EXCEPTION(std::logic_error{ - "invalid iterator"}); - } }; //------------------------------------------------------------------------------ @@ -265,9 +285,17 @@ const_iterator( : bn_(&bn) { if(! at_end) - next(C<0>{}); + { + it_.template emplace<1>( + net::buffer_sequence_begin( + detail::get<0>(*bn_))); + increment{*this}.next( + mp11::mp_size_t<1>{}); + } else - next(C{}); + { + it_.template emplace(); + } } template @@ -276,20 +304,7 @@ buffers_cat_view:: const_iterator:: operator==(const_iterator const& other) const { - return - (bn_ == nullptr) ? - ( - other.bn_ == nullptr || - other.it_.index() == sizeof...(Bn) + 1 - ):( - (other.bn_ == nullptr) ? - ( - it_.index() == sizeof...(Bn) + 1 - ): ( - bn_ == other.bn_ && - it_ == other.it_ - ) - ); + return bn_ == other.bn_ && it_ == other.it_; } template @@ -338,7 +353,10 @@ const_iterator:: operator--() -> const_iterator& { - decrement(C{}); + mp11::mp_with_index< + sizeof...(Bn) + 2>( + it_.index(), + decrement{*this}); return *this; } @@ -365,7 +383,6 @@ buffers_cat_view(Bn const&... bn) template -inline auto buffers_cat_view::begin() const -> const_iterator @@ -374,7 +391,6 @@ buffers_cat_view::begin() const -> } template -inline auto buffers_cat_view::end() const-> const_iterator diff --git a/test/beast/core/buffers_cat.cpp b/test/beast/core/buffers_cat.cpp index 0d5e4eba..73a4c05a 100644 --- a/test/beast/core/buffers_cat.cpp +++ b/test/beast/core/buffers_cat.cpp @@ -10,6 +10,8 @@ // Test that header file is self-contained. #include +#include "buffer_test.hpp" + #include #include #include @@ -26,213 +28,17 @@ namespace beast { class buffers_cat_test : public unit_test::suite { public: - template - static - std::reverse_iterator - make_reverse_iterator(Iterator i) - { - return std::reverse_iterator(i); - } - template static std::size_t - bsize1(ConstBufferSequence const& bs) + buffers_length( + ConstBufferSequence const& buffers) { - using net::buffer_size; - std::size_t n = 0; - for(auto it = bs.begin(); it != bs.end(); ++it) - n += buffer_size(*it); - return n; + return std::distance( + net::buffer_sequence_begin(buffers), + net::buffer_sequence_end(buffers)); } - template - static - std::size_t - bsize2(ConstBufferSequence const& bs) - { - using net::buffer_size; - std::size_t n = 0; - for(auto it = bs.begin(); it != bs.end(); it++) - n += buffer_size(*it); - return n; - } - - template - static - std::size_t - bsize3(ConstBufferSequence const& bs) - { - using net::buffer_size; - std::size_t n = 0; - for(auto it = bs.end(); it != bs.begin();) - n += buffer_size(*--it); - return n; - } - - template - static - std::size_t - bsize4(ConstBufferSequence const& bs) - { - using net::buffer_size; - std::size_t n = 0; - for(auto it = bs.end(); it != bs.begin();) - { - it--; - n += buffer_size(*it); - } - return n; - } - - void testBufferCat() - { - using net::buffer_size; - using net::const_buffer; - char buf[10]; - std::list b1; - std::vector b2{ - const_buffer{buf+0, 1}, - const_buffer{buf+1, 2}}; - std::list b3; - std::array b4{{ - const_buffer{buf+3, 1}, - const_buffer{buf+4, 2}, - const_buffer{buf+6, 3}}}; - std::list b5{ - const_buffer{buf+9, 1}}; - std::list b6; - auto bs = buffers_cat( - b1, b2, b3, b4, b5, b6); - BEAST_EXPECT(buffer_size(bs) == 10); - BEAST_EXPECT(bsize1(bs) == 10); - BEAST_EXPECT(bsize2(bs) == 10); - BEAST_EXPECT(bsize3(bs) == 10); - BEAST_EXPECT(bsize4(bs) == 10); - std::vector v; - for(auto iter = make_reverse_iterator(bs.end()); - iter != make_reverse_iterator(bs.begin()); ++iter) - v.emplace_back(*iter); - BEAST_EXPECT(buffer_size(bs) == 10); - decltype(bs) bs2(bs); - auto bs3(std::move(bs)); - { - net::streambuf sb1, sb2; - BEAST_EXPECT(buffer_size(buffers_cat( - sb1.prepare(5), sb2.prepare(7))) == 12); - sb1.commit(5); - sb2.commit(7); - BEAST_EXPECT(buffer_size(buffers_cat( - sb1.data(), sb2.data())) == 12); - } - for(auto it = bs.begin(); it != bs.end(); ++it) - { - decltype(bs)::const_iterator copy; - copy = it; - BEAST_EXPECT(copy == it); - auto copy2 = copy; - BEAST_EXPECT(copy2 == it); - } - } - - void testIterators() - { - using net::buffer_size; - using net::const_buffer; - char buf[9]; - std::vector b1{ - const_buffer{buf+0, 1}, - const_buffer{buf+1, 2}}; - std::array b2{{ - const_buffer{buf+3, 1}, - const_buffer{buf+4, 2}, - const_buffer{buf+6, 3}}}; - auto bs = buffers_cat(b1, b2); - for(int n = 0; - n <= std::distance(bs.begin(), bs.end()); ++n) - { - auto it = std::next(bs.begin(), n); - decltype(it) it2(std::move(it)); - it = std::move(it2); - auto pit = ⁢ - it = std::move(*pit); - } - try - { - std::size_t n = 0; - for(auto it = bs.begin(); n < 100; ++it) - ++n; - fail(); - } - catch(std::exception const&) - { - pass(); - } - - // decrement iterator - /* VFALCO - This causes a mysterious "uninitialized variable" - warning related to this function (see comment) - https://code.woboq.org/qt5/include/c++/6.3.1/bits/stl_iterator.h.html#159 - */ -#if 0 - { - auto const rbegin = - make_reverse_iterator(bs.end()); - auto const rend = - make_reverse_iterator(bs.begin()); - std::size_t n = 0; - for(auto it = rbegin; it != rend; ++it) - n += buffer_size(*it); - BEAST_EXPECT(n == 9); - } -#endif - - try - { - std::size_t n = 0; - for(auto it = bs.end(); n < 100; --it) - ++n; - fail(); - } - catch(std::exception const&) - { - pass(); - } - - try - { - buffer_size(*bs.end()); - fail(); - } - catch(std::exception const&) - { - pass(); - } - auto bs2 = bs; - BEAST_EXPECT(bs.begin() != bs2.begin()); - BEAST_EXPECT(bs.end() != bs2.end()); - decltype(bs)::const_iterator it; - decltype(bs2)::const_iterator it2; - BEAST_EXPECT(it == it2); - - // decrement begin() should throw - try - { - --bs.begin(); - fail(); - } - catch(std::logic_error const&) - { - pass(); - } - catch(...) - { - fail(); - } - } - - void testDefaultIterators() { @@ -275,6 +81,209 @@ public: } } + void + testBufferSequence() + { + string_view s = "Hello, world!"; + net::const_buffer b1(s.data(), 6); + net::const_buffer b2( + s.data() + b1.size(), s.size() - b1.size()); + test_buffer_sequence( + *this, buffers_cat(b1, b2)); + } + + template + void + checkException(F&& f) + { + try + { + f(); + fail("missing exception", __FILE__, __LINE__); + } + catch(std::logic_error const&) + { + pass(); + } + catch(...) + { + fail("wrong exception", __FILE__, __LINE__); + } + } + + void + testExceptions() + { + net::const_buffer b1{"He", 2}; + net::const_buffer b2{"llo,", 4}; + net::const_buffer b3{" world!", 7}; + + auto const b = beast::buffers_cat(b1, b2, b3); + + // Dereferencing a default-constructed iterator + checkException( + [] + { + *(decltype(b)::const_iterator{}); + }); + + // Incrementing a default-constructed iterator + checkException( + [] + { + ++(decltype(b)::const_iterator{}); + }); + + // Decrementing a default-constructed iterator + checkException( + [] + { + --(decltype(b)::const_iterator{}); + }); + + // Decrementing an iterator to the beginning + checkException( + [&b] + { + --b.begin(); + }); + + // Dereferencing an iterator to the end + checkException( + [&b] + { + *b.end(); + }); + + // Incrementing an iterator to the end + checkException( + [&b] + { + ++b.end(); + }); + } + + void + testEmpty() + { + using net::buffer_size; + + struct empty_sequence + { + using value_type = net::const_buffer; + using const_iterator = value_type const*; + + const_iterator + begin() const noexcept + { + return &v_; + } + + const_iterator + end() const noexcept + { + return begin(); + } + + private: + value_type v_; + }; + + { + net::const_buffer b0{}; + net::const_buffer b1{"He", 2}; + net::const_buffer b2{"llo,", 4}; + net::const_buffer b3{" world!", 7}; + + { + auto const b = beast::buffers_cat(b0, b0); + BEAST_EXPECT(buffer_size(b) == 0); + BEAST_EXPECT(buffers_length(b) == 0); + } + { + auto const b = beast::buffers_cat(b0, b0, b0, b0); + BEAST_EXPECT(buffer_size(b) == 0); + BEAST_EXPECT(buffers_length(b) == 0); + } + { + auto const b = beast::buffers_cat(b1, b2, b3); + BEAST_EXPECT(beast::buffers_to_string(b) == "Hello, world!"); + BEAST_EXPECT(buffers_length(b) == 3); + test_buffer_sequence(*this, b); + } + { + auto const b = beast::buffers_cat(b0, b1, b2, b3); + BEAST_EXPECT(beast::buffers_to_string(b) == "Hello, world!"); + BEAST_EXPECT(buffers_length(b) == 3); + test_buffer_sequence(*this, b); + } + { + auto const b = beast::buffers_cat(b1, b0, b2, b3); + BEAST_EXPECT(beast::buffers_to_string(b) == "Hello, world!"); + BEAST_EXPECT(buffers_length(b) == 3); + test_buffer_sequence(*this, b); + } + { + auto const b = beast::buffers_cat(b1, b2, b0, b3); + BEAST_EXPECT(beast::buffers_to_string(b) == "Hello, world!"); + BEAST_EXPECT(buffers_length(b) == 3); + test_buffer_sequence(*this, b); + } + { + auto const b = beast::buffers_cat(b1, b2, b3, b0); + BEAST_EXPECT(beast::buffers_to_string(b) == "Hello, world!"); + BEAST_EXPECT(buffers_length(b) == 3); + test_buffer_sequence(*this, b); + } + } + + { + auto e1 = net::const_buffer{}; + auto b1 = std::array{{ + e1, + net::const_buffer{"He", 2}, + net::const_buffer{"l", 1} }}; + auto b2 = std::array{{ + net::const_buffer{"lo", 2}, + e1, + net::const_buffer{", ", 2} }}; + auto b3 = std::array{{ + net::const_buffer{"w", 1}, + net::const_buffer{"orld!", 5}, + e1 }}; + { + auto const b = beast::buffers_cat( + e1, b1, e1, b2, e1, b3, e1); + BEAST_EXPECT(beast::buffers_to_string(b) == "Hello, world!"); + BEAST_EXPECT(buffers_length(b) == 6); + } + } + + { + auto e1 = net::const_buffer{}; + auto e2 = empty_sequence{}; + auto b1 = std::array{{ + e1, + net::const_buffer{"He", 2}, + net::const_buffer{"l", 1} }}; + auto b2 = std::array{{ + net::const_buffer{"lo", 2}, + e1, + net::const_buffer{", ", 2} }}; + auto b3 = std::array{ + net::const_buffer{"w", 1}, + net::const_buffer{"orld!", 5}, + e1 + }; + { + auto const b = beast::buffers_cat( + e2, b1, e2, b2, e2, b3, e2); + BEAST_EXPECT(beast::buffers_to_string(b) == "Hello, world!"); + BEAST_EXPECT(buffers_length(b) == 6); + } + } + } + void testGccWarning1() { @@ -307,70 +316,20 @@ public: char out[64]; const_buffer buffers("Hello, world!", 13); std::size_t i = 3; - buffers_suffix cb{buffers}; + buffers_suffix cb{buffers}; cb.consume(i); - buffer_copy( - buffer(out), + net::buffer_copy( + net::buffer(out), buffers_cat(buffers_prefix(i, buffers), cb)); } - void - test_empty_buffer_sequences() - { - using net::buffer_size; - using net::const_buffer; - std::vector v1; - std::vector v2; - BEAST_EXPECT(buffer_size(buffers_cat(v1, v2)) == 0); - } - void run() override { - using net::const_buffer; - using net::const_buffer; - using net::mutable_buffer; - struct user_defined : mutable_buffer - { - }; - - // Check is_all_const_buffer_sequence - BOOST_STATIC_ASSERT( - detail::is_all_const_buffer_sequence::value); - BOOST_STATIC_ASSERT( - detail::is_all_const_buffer_sequence::value); - BOOST_STATIC_ASSERT( - detail::is_all_const_buffer_sequence::value); - BOOST_STATIC_ASSERT( - detail::is_all_const_buffer_sequence::value); - BOOST_STATIC_ASSERT( - ! detail::is_all_const_buffer_sequence::value); - - // Ensure that concatenating mutable buffer - // sequences results in a mutable buffer sequence - BOOST_STATIC_ASSERT(std::is_same< - mutable_buffer, - decltype(buffers_cat( - std::declval(), - std::declval(), - std::declval>() - ))::value_type>::value); - - // Ensure that concatenating mixed buffer - // sequences results in a const buffer sequence. - BOOST_STATIC_ASSERT(std::is_same< - const_buffer, - decltype(buffers_cat( - std::declval(), - std::declval(), - std::declval() - ))::value_type>::value); - - testBufferCat(); - testIterators(); - testDefaultIterators(); + testBufferSequence(); + testExceptions(); + testEmpty(); testGccWarning1(); testGccWarning2(); - test_empty_buffer_sequences(); } };