circular_buffer icon indicating copy to clipboard operation
circular_buffer copied to clipboard

Add emplace_{back,front} to circular_buffer - rebased to latest develop

Open f9z opened this issue 2 years ago • 8 comments

based on https://github.com/boostorg/circular_buffer/pull/15 request by AI0867

f9z avatar May 13 '22 02:05 f9z

@AI0867 i've taken your code rebased to develop and made minor adjustments to be compatible with latest changes. @gpascualg would highly appreciate if you could help me with review please. i used your proposed patch to avoid extra destructor call.

f9z avatar May 13 '22 02:05 f9z

@f9z Thank you for your efforts. I haven't had the time to set up a build environment on my current machine to do this myself.

AI0867 avatar May 16 '22 07:05 AI0867

Looks good to me, plus it's more consistent with the circular_buffer code than the patch I proposed. Thank you for putting it together.

gpascualg avatar May 16 '22 09:05 gpascualg

That'd be the procedure to merge it to "develop" from here @gpascualg ? It's my first PR to boostorg. Anything else I need to do to make it happen? Thanks!

f9z avatar May 16 '22 11:05 f9z

I'm afraid I can't really help you here, as I have never done a PR to any boost library. I'd guess, though, that this should be it.

gpascualg avatar May 16 '22 12:05 gpascualg

@glenfe would it be possible to consider merging this PR? please let me know if i'd need to do anything else or approach some other boostorg maintainers. thanks!

f9z avatar May 17 '22 04:05 f9z

what was the problem with the initial replace() code?

I left a comment about what I feel was observed here https://github.com/boostorg/circular_buffer/pull/15#issuecomment-1572194345

this is an excellent PR but I would sort out the situation and possibly discard the last commit from the PR...

lano1106 avatar Jun 01 '23 18:06 lano1106

Here is my take. This is a patch over branch after having reverted "replace replace() with destroy_item() + construct()"

This reverts commit c4c85d730dc8e31c4d50aa45ada146ce8c2f5340.

diff --git a/include/boost/circular_buffer/base.hpp b/include/boost/circular_buffer/base.hpp
index 5ed1321..846cb32 100644
--- a/include/boost/circular_buffer/base.hpp
+++ b/include/boost/circular_buffer/base.hpp
@@ -1460,7 +1460,7 @@ private:
         if (full()) {
             if (empty())
                 return;
-            replace(m_last, value_type(::boost::forward<Args>(args)...));
+            destructive_replace(m_last, ::boost::forward<Args>(args)...);
             increment(m_last);
             m_first = m_last;
         } else {
@@ -1470,16 +1470,16 @@ private:
         }
     }
 #else
-    template <class V>
-    void emplace_back_impl(BOOST_FWD_REF(V) value) {
+    template <class Arg>
+    void emplace_back_impl(BOOST_FWD_REF(Arg) arg) {
         if (full()) {
             if (empty())
                 return;
-            replace(m_last, value_type(::boost::forward<V>(value)));
+            destructive_replace(m_last, ::boost::forward<Arg>(arg));
             increment(m_last);
             m_first = m_last;
         } else {
-            boost::allocator_construct(alloc(), boost::to_address(m_last), ::boost::forward<V>(value));
+            boost::allocator_construct(alloc(), boost::to_address(m_last), ::boost::forward<Arg>(arg));
             increment(m_last);
             ++m_size;
         }
@@ -1494,7 +1494,7 @@ private:
                 if (empty())
                     return;
                 decrement(m_first);
-                replace(m_first, value_type(::boost::forward<Args>(args)...));
+                destructive_replace(m_first, ::boost::forward<Args>(args)...);
                 m_last = m_first;
             } else {
                 decrement(m_first);
@@ -1508,18 +1508,18 @@ private:
         BOOST_CATCH_END
     }
 #else
-    template <class V>
-    void emplace_front_impl(BOOST_FWD_REF(V) value) {
+    template <class Arg>
+    void emplace_front_impl(BOOST_FWD_REF(Arg) arg) {
         BOOST_TRY {
             if (full()) {
                 if (empty())
                     return;
                 decrement(m_first);
-                replace(m_first, value_type(::boost::forward<V>(value)));
+                destructive_replace(m_first, ::boost::forward<Arg>(arg));
                 m_last = m_first;
             } else {
                 decrement(m_first);
-                boost::allocator_construct(alloc(), boost::to_address(m_first), ::boost::forward<V>(value));
+                boost::allocator_construct(alloc(), boost::to_address(m_first), ::boost::forward<Arg>(arg));
                 ++m_size;
             }
         } BOOST_CATCH(...) {
@@ -2560,6 +2560,30 @@ private:
 #endif
     }
 
+#if !defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES)
+    template<class... Args>
+    void destructive_replace(pointer pos, BOOST_FWD_REF(Args)... args)
+    {
+        boost::allocator_destroy(alloc(), boost::to_address(pos));
+        boost::allocator_construct(alloc(), boost::to_address(pos),
+                                   ::boost::forward<Args>(args)...);
+#if BOOST_CB_ENABLE_DEBUG
+        invalidate_iterators(iterator(this, pos));
+#endif
+    }
+#else
+    template <class Arg>
+    void destructive_replace(pointer pos, BOOST_FWD_REF(Arg) arg)
+    {
+        boost::allocator_destroy(alloc(), boost::to_address(pos));
+        boost::allocator_construct(alloc(), boost::to_address(pos),
+                                   ::boost::forward<Arg>(arg));
+#if BOOST_CB_ENABLE_DEBUG
+        invalidate_iterators(iterator(this, pos));
+#endif
+    }
+#endif
+
     /*! INTERNAL ONLY */
     void construct_or_replace(bool construct, pointer pos, param_value_type item) {
         if (construct)

lano1106 avatar Jun 02 '23 17:06 lano1106