adt - make circular buffer work with types without default ctor

lib/include/srsran/adt/circular_buffer.h

@@ -13,7 +13,9 @@
 #ifndef SRSRAN_CIRCULAR_BUFFER_H
 #define SRSRAN_CIRCULAR_BUFFER_H
 
+#include "srsran/adt/detail/type_storage.h"
 #include "srsran/adt/expected.h"
+#include "srsran/common/srsran_assert.h"
 
 #include <array>
 #include <cassert>
@@ -51,51 +53,97 @@ size_t get_max_size(const std::vector<T>& a)
 template <typename Container>
 class base_circular_buffer
 {
-  using T = typename Container::value_type;
+  using storage_t = typename Container::value_type;
+  using T         = typename storage_t::value_type;
 
-public:
+  template <typename DataType>
-  using value_type      = T;
+  class iterator_impl
-  using difference_type = typename Container::difference_type;
+  {
+    using parent_type = typename std::conditional<std::is_same<DataType, T>::value,
+                                                  base_circular_buffer<Container>,
+                                                  const base_circular_buffer<Container> >::type;
+
+  public:
+    using value_type        = DataType;
+    using reference         = DataType&;
+    using pointer           = DataType*;
+    using difference_type   = std::ptrdiff_t;
+    using iterator_category = std::forward_iterator_tag;
 
-  struct iterator {
+    iterator_impl(parent_type& parent_, size_t i) : parent(&parent_), idx(i) {}
-    iterator(base_circular_buffer<Container>& parent_, size_t i) : parent(&parent_), idx(i) {}
+
-    iterator& operator++()
+    iterator_impl<DataType>& operator++()
     {
       idx = (idx + 1) % parent->max_size();
       return *this;
     }
-    iterator operator++(int)
+    iterator_impl<DataType> operator++(int)
     {
-      iterator tmp(*this);
+      iterator_impl<DataType> tmp(*this);
       ++(*this);
       return tmp;
     }
-    iterator operator+(difference_type n)
+    iterator_impl<DataType> operator+(difference_type n)
     {
-      iterator tmp(*this);
+      iterator_impl<DataType> tmp(*this);
       tmp += n;
       return tmp;
     }
-    iterator& operator+=(difference_type n)
+    iterator_impl<DataType>& operator+=(difference_type n)
     {
       idx = (idx + n) % parent->max_size();
       return *this;
     }
-    value_type*       operator->() { return &parent->buffer[idx]; }
+    value_type*       operator->() { return &get(); }
-    const value_type* operator->() const { return &parent->buffer[idx]; }
+    const value_type* operator->() const { return &get(); }
-    value_type&       operator*() { return parent->buffer[idx]; }
+    value_type&       operator*() { return get(); }
-    const value_type& operator*() const { return parent->buffer[idx]; }
+    const value_type& operator*() const { return get(); }
-    bool              operator==(const iterator& it) const { return it.parent == parent and it.idx == idx; }
+
-    bool              operator!=(const iterator& it) const { return not(*this == it); }
+    bool operator==(const iterator_impl<DataType>& it) const { return it.parent == parent and it.idx == idx; }
+    bool operator!=(const iterator_impl<DataType>& it) const { return not(*this == it); }
 
   private:
-    base_circular_buffer<Container>* parent;
+    void assert_idx_within_bounds()
+    {
+      srsran_assert(idx + (idx >= parent->rpos ? 0 : parent->max_size()) < parent->rpos + parent->count,
+                    "index=%zd is out-of-bounds [%zd, %zd)",
+                    idx,
+                    parent->rpos,
+                    parent->count);
+    }
+    value_type& get()
+    {
+      assert_idx_within_bounds();
+      return parent->buffer[idx].get();
+    }
+    const value_type& get() const
+    {
+      assert_idx_within_bounds();
+      return parent->buffer[idx].get();
+    }
+    parent_type* parent;
     size_t       idx;
   };
 
-  template <typename... Args>
+public:
-  explicit base_circular_buffer(Args&&... args) : buffer(std::forward<Args>(args)...)
+  using value_type      = T;
-  {}
+  using difference_type = typename Container::difference_type;
+  using size_type       = std::size_t;
+
+  using iterator       = iterator_impl<T>;
+  using const_iterator = iterator_impl<const T>;
+
+  base_circular_buffer() = default;
+  ~base_circular_buffer() { clear(); }
+
+  template <typename U>
+  typename std::enable_if<std::is_constructible<T, U>::value>::type push(U&& t)
+  {
+    srsran_assert(not full(), "Circular buffer is full.");
+    size_t wpos = (rpos + count) % max_size();
+    buffer[wpos].emplace(std::forward<U>(t));
+    count++;
+  }
 
   bool try_push(T&& t)
   {
@@ -104,13 +152,7 @@ public:
     }
     push(std::move(t));
   }
-  void push(T&& t)
+
-  {
-    assert(not full());
-    size_t wpos  = (rpos + count) % max_size();
-    buffer[wpos] = std::move(t);
-    count++;
-  }
   bool try_push(const T& t)
   {
     if (full()) {
@@ -118,49 +160,56 @@ public:
     }
     push(t);
   }
-  void push(const T& t)
-  {
-    assert(not full());
-    size_t wpos  = (rpos + count) % max_size();
-    buffer[wpos] = t;
-    count++;
-  }
   void pop()
   {
-    assert(not empty());
+    srsran_assert(not empty(), "Cannot call pop() in empty circular buffer");
+    buffer[rpos].destroy();
     rpos = (rpos + 1) % max_size();
     count--;
   }
   T& top()
   {
-    assert(not empty());
+    srsran_assert(not empty(), "Cannot call top() in empty circular buffer");
-    return buffer[rpos];
+    return buffer[rpos].get();
   }
   const T& top() const
   {
-    assert(not empty());
+    srsran_assert(not empty(), "Cannot call top() in empty circular buffer");
-    return buffer[rpos];
+    return buffer[rpos].get();
+  }
+  void clear()
+  {
+    for (size_t i = 0; i < count; ++i) {
+      buffer[rpos + i].destroy();
+    }
+    count = 0;
   }
-  void clear() { count = 0; }
 
   bool   full() const { return count == max_size(); }
   bool   empty() const { return count == 0; }
   size_t size() const { return count; }
   size_t max_size() const { return detail::get_max_size(buffer); }
 
-  iterator begin() { return iterator(*this, rpos); }
+  T& operator[](size_t i)
-  iterator end() { return iterator(*this, (rpos + count) % max_size()); }
-
-  template <typename = std::enable_if<std::is_same<Container, std::vector<T> >::value> >
-  void set_size(size_t size)
   {
-    buffer.resize(size);
+    srsran_assert(i < count, "Out-of-bounds access to circular buffer (%zd >= %zd)", i, count);
+    return buffer[(rpos + i) % max_size()].get();
   }
+  const T& operator[](size_t i) const
+  {
+    srsran_assert(i < count, "Out-of-bounds access to circular buffer (%zd >= %zd)", i, count);
+    return buffer[(rpos + i) % max_size()].get();
+  }
+
+  iterator       begin() { return iterator(*this, rpos); }
+  const_iterator begin() const { return const_iterator(*this, rpos); }
+  iterator       end() { return iterator(*this, (rpos + count) % max_size()); }
+  const_iterator end() const { return const_iterator(*this, (rpos + count) % max_size()); }
 
   template <typename F>
   T discard_if(const F& func)
   {
-    for (auto it = buffer.begin(); it != buffer.end(); it++) {
+    for (auto it = begin(); it != end(); it++) {
       if (*it != nullptr && func(*it)) {
         T tmp = std::move(*it);
         *it   = nullptr;
@@ -170,7 +219,13 @@ public:
     return nullptr;
   }
 
-private:
+protected:
+  base_circular_buffer(size_t rpos_, size_t count_) : rpos(rpos_), count(count_) {}
+  template <typename... BufferArgs>
+  base_circular_buffer(size_t rpos_, size_t count_, BufferArgs&&... args) :
+    rpos(rpos_), count(count_), buffer(std::forward<BufferArgs>(args)...)
+  {}
+
   Container buffer;
   size_t    rpos  = 0;
   size_t    count = 0;
@@ -394,8 +449,53 @@ protected:
  * @tparam N size of the queue
  */
 template <typename T, size_t N>
-class static_circular_buffer : public detail::base_circular_buffer<std::array<T, N> >
+class static_circular_buffer : public detail::base_circular_buffer<std::array<detail::type_storage<T>, N> >
-{};
+{
+  using base_t = detail::base_circular_buffer<std::array<detail::type_storage<T>, N> >;
+
+public:
+  static_circular_buffer() = default;
+  static_circular_buffer(const static_circular_buffer& other) : base_t(other.rpos, other.count)
+  {
+    static_assert(std::is_copy_constructible<T>::value, "T must be copy-constructible");
+    std::uninitialized_copy(other.begin(), other.end(), base_t::begin());
+  }
+  static_circular_buffer(static_circular_buffer<T, N>&& other) noexcept : base_t(other.rpos, other.count)
+  {
+    static_assert(std::is_move_constructible<T>::value, "T must be move-constructible");
+    for (size_t i = 0; i < other.count; ++i) {
+      size_t idx = (other.rpos + i) % other.max_size();
+      base_t::buffer[idx].move_ctor(std::move(other.buffer[idx]));
+    }
+    other.clear();
+  }
+  static_circular_buffer& operator=(const static_circular_buffer& other)
+  {
+    if (this == &other) {
+      return *this;
+    }
+    base_t::clear();
+    base_t::rpos  = other.rpos;
+    base_t::count = other.count;
+    for (size_t i = 0; i < other.count; ++i) {
+      size_t idx = (other.rpos + i) % other.max_size();
+      base_t::buffer[idx].copy_ctor(other.buffer[idx]);
+    }
+    return *this;
+  }
+  static_circular_buffer& operator=(static_circular_buffer&& other) noexcept
+  {
+    base_t::clear();
+    base_t::rpos  = other.rpos;
+    base_t::count = other.count;
+    for (size_t i = 0; i < other.count; ++i) {
+      size_t idx = (other.rpos + i) % other.max_size();
+      base_t::buffer[idx].move_ctor(std::move(other.buffer[idx]));
+    }
+    other.clear();
+    return *this;
+  }
+};
 
 /**
  * Circular buffer with buffer storage via a std::vector<T>.
@@ -404,19 +504,44 @@ class static_circular_buffer : public detail::base_circular_buffer<std::array<T,
  * @tparam T value type stored by buffer
  */
 template <typename T>
-class dyn_circular_buffer : public detail::base_circular_buffer<std::vector<T> >
+class dyn_circular_buffer : public detail::base_circular_buffer<std::vector<detail::type_storage<T> > >
 {
-  using base_t = detail::base_circular_buffer<std::vector<T> >;
+  using base_t = detail::base_circular_buffer<std::vector<detail::type_storage<T> > >;
 
 public:
   dyn_circular_buffer() = default;
-  explicit dyn_circular_buffer(size_t size) : base_t(size) {}
+  explicit dyn_circular_buffer(size_t max_size) : base_t(0, 0, max_size) {}
+  dyn_circular_buffer(dyn_circular_buffer&& other) noexcept : base_t(other.rpos, other.count, std::move(other.buffer))
+  {
+    other.count = 0;
+    other.rpos  = 0;
+  }
+  dyn_circular_buffer(const dyn_circular_buffer& other) : base_t(other.rpos, other.count, other.max_size())
+  {
+    static_assert(std::is_copy_constructible<T>::value, "T must be copy-constructible");
+    for (size_t i = 0; i < other.count; ++i) {
+      size_t idx = (other.rpos + i) % other.max_size();
+      base_t::buffer[idx].copy_ctor(other.buffer[idx]);
+    }
+  }
+  dyn_circular_buffer& operator=(dyn_circular_buffer other) noexcept
+  {
+    swap(other);
+    other.clear();
+    return *this;
+  }
+
+  void swap(dyn_circular_buffer& other) noexcept
+  {
+    std::swap(base_t::rpos, other.rpos);
+    std::swap(base_t::count, other.count);
+    std::swap(base_t::buffer, other.buffer);
+  }
 
   void set_size(size_t size)
   {
-    // Note: dynamic resizes not supported.
+    srsran_assert(base_t::empty(), "Dynamic resizes not supported when circular buffer is not empty");
-    assert(base_t::empty());
+    base_t::buffer.resize(size);
-    base_t::set_size(size);
   }
 };
 

lib/include/srsran/adt/circular_map.h

@@ -105,7 +105,7 @@ public:
   {
     for (size_t idx = 0; idx < other.capacity(); ++idx) {
       if (present[idx]) {
-        buffer[idx].template construct(other.get_obj_(idx));
+        buffer[idx].template emplace(other.get_obj_(idx));
       }
     }
   }
@@ -113,7 +113,7 @@ public:
   {
     for (size_t idx = 0; idx < other.capacity(); ++idx) {
       if (present[idx]) {
-        buffer[idx].template construct(std::move(other.get_obj_(idx)));
+        buffer[idx].template emplace(std::move(other.get_obj_(idx)));
       }
     }
     other.clear();
@@ -153,7 +153,7 @@ public:
     if (present[idx]) {
       return false;
     }
-    buffer[idx].template construct(id, obj);
+    buffer[idx].template emplace(id, obj);
     present[idx] = true;
     count++;
     return true;
@@ -164,7 +164,7 @@ public:
     if (present[idx]) {
       return srsran::expected<iterator, T>(std::move(obj));
     }
-    buffer[idx].template construct(id, std::move(obj));
+    buffer[idx].template emplace(id, std::move(obj));
     present[idx] = true;
     count++;
     return iterator(this, idx);
@@ -240,7 +240,7 @@ private:
   obj_t&       get_obj_(size_t idx) { return buffer[idx].get(); }
   const obj_t& get_obj_(size_t idx) const { return buffer[idx].get(); }
 
-  std::array<type_storage<obj_t>, N> buffer;
+  std::array<detail::type_storage<obj_t>, N> buffer;
   std::array<bool, N>                        present;
   size_t                                     count = 0;
 };

lib/include/srsran/adt/detail/type_storage.h

@@ -18,23 +18,21 @@
 
 namespace srsran {
 
+namespace detail {
+
 template <typename T>
 struct type_storage {
+  using value_type = T;
+
   template <typename... Args>
-  void construct(Args&&... args)
+  void emplace(Args&&... args)
   {
     new (&buffer) T(std::forward<Args>(args)...);
   }
   void destroy() { get().~T(); }
-  void copy_ctor(const type_storage<T>& other) { buffer.get() = other.get(); }
+  void copy_ctor(const type_storage<T>& other) { emplace(other.get()); }
-  void move_ctor(type_storage<T>&& other) { buffer.get() = std::move(other.get()); }
+  void move_ctor(type_storage<T>&& other) { emplace(std::move(other.get())); }
-  void copy_assign(const type_storage<T>& other)
+  void copy_assign(const type_storage<T>& other) { get() = other.get(); }
-  {
-    if (this == &other) {
-      return;
-    }
-    get() = other.get();
-  }
   void move_assign(type_storage<T>&& other) { get() = std::move(other.get()); }
 
   T&       get() { return reinterpret_cast<T&>(buffer); }
@@ -53,7 +51,7 @@ void copy_if_present_helper(type_storage<T>& lhs, const type_storage<T>& rhs, bo
     lhs.destroy();
   }
   if (rhs_present) {
-    lhs.template construct(rhs.get());
+    lhs.copy_ctor(rhs);
   }
 }
 
@@ -67,10 +65,12 @@ void move_if_present_helper(type_storage<T>& lhs, type_storage<T>& rhs, bool lhs
     lhs.destroy();
   }
   if (rhs_present) {
-    lhs.template construct(std::move(rhs.get()));
+    lhs.move_ctor(std::move(rhs));
   }
 }
 
+} // namespace detail
+
 } // namespace srsran
 
 #endif // SRSRAN_TYPE_STORAGE_H

lib/include/srsran/common/srsran_assert.h

@@ -28,8 +28,8 @@
   do {                                                                                                                 \
     if (srsran_unlikely(not(condition))) {                                                                             \
       srslog::fetch_basic_logger("ALL").error("%s:%d: " fmt, __FILE__, __LINE__, ##__VA_ARGS__);                       \
-      srsran::console_stderr("%s:%d: " fmt "\n", __FILE__, __LINE__, ##__VA_ARGS__);                                   \
       srslog::flush();                                                                                                 \
+      srsran::console_stderr("%s:%d: " fmt "\n", __FILE__, __LINE__, ##__VA_ARGS__);                                   \
       std::abort();                                                                                                    \
     }                                                                                                                  \
   } while (0)

lib/test/adt/circular_buffer_test.cc

@@ -15,8 +15,33 @@
 
 namespace srsran {
 
+struct C {
+  C() : val_ptr(new int(5)) { count++; }
+  ~C() { count--; }
+  C(C&& other) : val_ptr(move(other.val_ptr)) { count++; }
+  C& operator=(C&&) = default;
+
+  std::unique_ptr<int> val_ptr;
+
+  static size_t count;
+};
+size_t C::count = 0;
+
+struct D {
+  D() { count++; }
+  ~D() { count--; }
+  D(const D&) { count++; }
+  D(D&&)     = delete;
+  D& operator=(D&&) = delete;
+  D& operator=(const D&) = default;
+
+  static size_t count;
+};
+size_t D::count = 0;
+
 int test_static_circular_buffer()
 {
+  {
     static_circular_buffer<int, 10> circ_buffer;
     TESTASSERT(circ_buffer.max_size() == 10);
     TESTASSERT(circ_buffer.empty() and not circ_buffer.full() and circ_buffer.size() == 0);
@@ -31,7 +56,7 @@ int test_static_circular_buffer()
 
     // test iterator
     int count = 0;
-  for (int& it : circ_buffer) {
+    for (int it : circ_buffer) {
       TESTASSERT(it == count);
       count++;
     }
@@ -60,10 +85,128 @@ int test_static_circular_buffer()
       TESTASSERT(it == count);
       count++;
     }
+  }
 
+  // TEST: move-only types
+  {
+    static_circular_buffer<C, 5> circbuffer;
+    circbuffer.push(C{});
+    circbuffer.push(C{});
+    circbuffer.push(C{});
+    circbuffer.push(C{});
+    circbuffer.push(C{});
+    TESTASSERT(circbuffer.full() and C::count == 5);
+    C c = std::move(circbuffer.top());
+    TESTASSERT(circbuffer.full() and C::count == 6);
+    circbuffer.pop();
+    TESTASSERT(not circbuffer.full() and C::count == 5);
+
+    static_circular_buffer<C, 5> circbuffer2(std::move(circbuffer));
+    TESTASSERT(circbuffer.empty() and circbuffer2.size() == 4);
+    TESTASSERT(C::count == 5);
+    circbuffer.push(C{});
+    TESTASSERT(C::count == 6);
+    circbuffer = std::move(circbuffer2);
+    TESTASSERT(C::count == 5);
+  }
+
+  TESTASSERT(C::count == 0);
   return SRSRAN_SUCCESS;
 }
 
+void test_dyn_circular_buffer()
+{
+  {
+    dyn_circular_buffer<int> circ_buffer(10);
+    TESTASSERT(circ_buffer.max_size() == 10);
+    TESTASSERT(circ_buffer.empty() and not circ_buffer.full() and circ_buffer.size() == 0);
+
+    // push until full
+    for (size_t i = 0; i < circ_buffer.max_size(); ++i) {
+      TESTASSERT(circ_buffer.size() == i and not circ_buffer.full());
+      circ_buffer.push(i);
+      TESTASSERT(not circ_buffer.empty());
+    }
+    TESTASSERT(circ_buffer.size() == 10 and circ_buffer.full());
+
+    // test iterator
+    int count = 0;
+    for (int it : circ_buffer) {
+      TESTASSERT(it == count);
+      count++;
+    }
+    TESTASSERT(*circ_buffer.begin() == circ_buffer.top());
+
+    // pop until empty
+    for (size_t i = 0; i < circ_buffer.max_size(); ++i) {
+      TESTASSERT(circ_buffer.size() == circ_buffer.max_size() - i and not circ_buffer.empty());
+      TESTASSERT(circ_buffer.top() == (int)i);
+      circ_buffer.pop();
+    }
+    TESTASSERT(circ_buffer.empty() and circ_buffer.size() == 0);
+
+    // test iteration with wrap-around in memory
+    for (size_t i = 0; i < circ_buffer.max_size(); ++i) {
+      circ_buffer.push(i);
+    }
+    for (size_t i = 0; i < circ_buffer.max_size() / 2; ++i) {
+      circ_buffer.pop();
+    }
+    circ_buffer.push(circ_buffer.max_size());
+    circ_buffer.push(circ_buffer.max_size() + 1);
+    TESTASSERT(circ_buffer.size() == circ_buffer.max_size() / 2 + 2);
+    count = circ_buffer.max_size() / 2;
+    for (int& it : circ_buffer) {
+      TESTASSERT(it == count);
+      count++;
+    }
+  }
+
+  // TEST: move-only types
+  {
+    dyn_circular_buffer<C> circbuffer(5);
+    circbuffer.push(C{});
+    circbuffer.push(C{});
+    circbuffer.push(C{});
+    circbuffer.push(C{});
+    circbuffer.push(C{});
+    TESTASSERT(circbuffer.full() and C::count == 5);
+    C c = std::move(circbuffer.top());
+    TESTASSERT(circbuffer.full() and C::count == 6);
+    circbuffer.pop();
+    TESTASSERT(not circbuffer.full() and C::count == 5);
+
+    dyn_circular_buffer<C> circbuffer2(std::move(circbuffer));
+    TESTASSERT(circbuffer.empty() and circbuffer2.size() == 4);
+    TESTASSERT(C::count == 5);
+    circbuffer.set_size(5);
+    circbuffer.push(C{});
+    TESTASSERT(C::count == 6);
+    circbuffer = std::move(circbuffer2);
+    TESTASSERT(C::count == 5);
+  }
+
+  // TEST: copy-only types
+  {
+    dyn_circular_buffer<D> circbuffer(3);
+    D                      d{};
+    circbuffer.push(d);
+    circbuffer.push(d);
+    circbuffer.push(d);
+    TESTASSERT(circbuffer.full() and D::count == 4);
+
+    dyn_circular_buffer<D> circbuffer2(circbuffer);
+    TESTASSERT(circbuffer2.full() and circbuffer.full());
+    TESTASSERT(D::count == 7);
+    circbuffer.pop();
+    circbuffer.pop();
+    TESTASSERT(D::count == 5);
+    circbuffer = circbuffer2;
+    TESTASSERT(D::count == 7);
+  }
+  TESTASSERT(C::count == 0);
+}
+
 int test_queue_block_api()
 {
   dyn_blocking_queue<int> queue(100);
@@ -115,9 +258,15 @@ int test_queue_block_api_2()
 
 } // namespace srsran
 
-int main()
+int main(int argc, char** argv)
 {
+  auto& test_log = srslog::fetch_basic_logger("TEST");
+  test_log.set_level(srslog::basic_levels::info);
+
+  srsran::test_init(argc, argv);
+
   TESTASSERT(srsran::test_static_circular_buffer() == SRSRAN_SUCCESS);
+  srsran::test_dyn_circular_buffer();
   TESTASSERT(srsran::test_queue_block_api() == SRSRAN_SUCCESS);
   TESTASSERT(srsran::test_queue_block_api_2() == SRSRAN_SUCCESS);
   srsran::console("Success\n");