Simple ring/circular buffer c++ class V3

Question

The Original Post (v2)

As stated in the previous post, i'm looking for feedback on the updated version of the code posted.

A few things specifically I'm looking for feedback on, are:

• I switched to using the std::aligned_storage as suggested in the previous post, but on the cppreference.com - std::aligned_storage page, in the examples it says // note: needs std::launder as of C++17 above the reinterpret_cast's. Would that mean, that the launder function should just wrap the cast, and if so, it states that its needed for c++17, which is what I'm using, but I'd also like the code to be compatible back to c++11. What would be the recommended approach in this situation?

• I was considering making a private function, something like:

  #include <new>
  // ...
  [[nodiscard]] const T* ptr_to(std::size_t pos) const {
   return std::launder(reinterpret_cast<const T*>(&data[pos]));
  }
  

  to remove some of the code duplication, I'm still trying to gain a better understanding of const correctness, would I need a const version and a non-const version of this function, similar to the peek function?

• Like I mentioned in a previous point, I'm wanting to target c++17 primarily, but also be backward compatible down to c++11. Language features such as [[nodiscard]] and std::launder are c++17 only, is there a standard way(using macros or otherwise) to accommodate for this?

• The function void push(const T& item) noexcept takes a const ref and uses std::move internally on it, is that reasonable? or should that only be done to params such as T&& item? Should I just delete the std::move in that function?

• Any suggestions for cutting down on code duplication are welcome.

CircularBuffer.h

#pragma once
#include <cassert>
#include <type_traits>
namespace datastructures {
 template<class T, std::size_t N>
 class CircularBuffer {
 typename std::aligned_storage<sizeof(T), alignof(T)>::type data[N];
 std::size_t head = 0;
 std::size_t tail = 0;
 bool isFull = false;
 public:
 template<typename ...Args>
 void emplace_push(Args&&... args) noexcept {
 assert(!isFull && "Attempting to push item into full buffer!");
 new (&data[head]) T(std::forward<Args>(args)...);
 head = ++head % N;
 isFull = head == tail;
 }
 void push(const T& item) noexcept {
 assert(!isFull && "Attempting to push item into full buffer!");
 new (&data[head]) T(std::move(item));
 head = ++head % N;
 isFull = head == tail;
 }
 T pop() noexcept {
 assert(!is_empty() && "Attempting to pop item from empty buffer!");
 auto ptr = reinterpret_cast<T*>(&data[tail]);
 auto result = std::move(*ptr);
 ptr->~T();
 tail = ++tail % N;
 isFull = false;
 return result;
 }
 [[nodiscard]] constexpr T& peek() noexcept {
 assert(!is_empty() && "Attempting to peek in empty buffer!");
 return *reinterpret_cast<T*>(&data[tail]);
 }
 [[nodiscard]] constexpr const T& peek() const noexcept {
 assert(!is_empty() && "Attempting to peek in empty buffer!");
 return *reinterpret_cast<const T*>(&data[tail]);
 }
 [[nodiscard]] constexpr bool is_empty() const noexcept {
 return !isFull && tail == head;
 }
 [[nodiscard]] constexpr std::size_t get_capacity() const noexcept {
 return N;
 }
 [[nodiscard]] constexpr std::size_t get_size() const noexcept {
 if (isFull)
 return N;
 if (head >= tail)
 return head - tail;
 return N + head - tail;
 }
 };
}

---

Edit: I started using the macro:

#ifdef __has_cpp_attribute
# if __has_cpp_attribute(nodiscard)
# define NO_DISCARD [[nodiscard]]
# else
# define NO_DISCARD
# endif
#else
# define NO_DISCARD
#endif

for the [[nodiscard]] on the functions. Is this reasonably portable / acceptable or is there anything better, also still looking for a good way to conditionally add the std::launder part if available in the language.

Answer 1

You say you want to

target c++17 primarily, but also be backward compatible down to c++11.

Consider changing this to

target c++11 primarily, but also be compatible with c++17

Because you can't really use most of the c++17 stuff here. You have to write code in c++11, but also take care of c++17 compatibility. To be honest, I don't think this is a good idea, but I will respect your opinion anyway.

I don't know much about std::launder, but I remember someone saying that the code with theoretical undefined behavior will do the right job on any (sane) implementation. I'm not gonna care much about it here.

Bugs

1. Your code does not implement RAII and is not exception-safe. You should add a destructor to clean up the contents. You should also implement (copy|move) (constructors|assignments). The copy-and-swap idiom can help you.

   Note that the automatically generated destructor does not call the destructor of T, and the automatically generated copy and move stuff do not call the copy and move stuff of T. They just copy the raw chars, which is clearly not intended.

   Here's an example of what a destructor might look like:

   ~CircularBuffer()
   {
    if (is_empty())
    return;
    std::size_t i = tail;
    do {
    reinterpret_cast<T*>(&data[i])->~T();
    i = (i + 1) % N;
    } while (i != head);
   }
   

   And the copy constructor and move constructor:

   CircularBuffer(const CircularBuffer& other)
    :head{other.head}, tail{other.tail}, isFull{other.isFull}
   {
    // note: not exception safe
    if (!is_empty()) {
    for (; i != head; i = (i + 1) % N)
    new (&data[i]) T(other.data[i]);
    }
   }
   CircularBuffer(CircularBuffer&& other)
    :head{other.head}, tail{other.tail}, isFull{other.isFull}
   {
    // note: not exception safe
    if (!is_empty())
    for (std::size_t i = tail; i < head; i = (i + 1) % N)
    new (&data[i]) T(std::move(other.data[i]));
   }
   

Suggestions

1. The following code is confusing:

   head = ++head % N;
   

   Granted, it works as intended since c++11, but still, consider changing to the more readable

   head = (head + 1) % N;
   

   This will prevent any confusion.

2. push accepts a const T& and std::moves from it. This makes no sense. std::move(item) will return a const T&& and usually binds to const T& copy constructors. Pass-by-value instead:

   void push(T item) noexcept { /* ... */ }
   

   This will handle both copying and moving correctly.

3. Moreover, push should probably delegate to emplace_push to reduce code duplication.

   void push(T item) noexcept
   {
    emplace_push(std::move(item));
   }
   

4. pop should not return the popped value because it cannot do this in a safe way.¹ Instead, let pop return void and let peek do its job.

By the way, there is much debate on #pragma once (see, for example, #pragma once vs include guards?). I think it is OK here, but others may disagree.

---

¹ The move constructor of T may throw an exception, in which case the popped object is leaked and cannot be restored.

• Hex: So I've made almost all the changes mentioned aside from the constructor related ones, firstly, are copy/move/etc constructors not automatically generated if no constructor is defined? I thought they were? Aside from that, how are template parameters handled when dealing with copy constructors etc, would you be able to give a very basic example with at least the aligned storage member variable? Thanks.
• L. F.: @Hex I have updated my answer to answer your first question. For the example, wait a moment
• Hex: I also added a "move push" ` void push(T&& item) noexcept { assert(!isFull && "error: buffer is full!"); new (&data[head]) T(std::move(item)); increment_head(); }` is it proper to use the std::move here or is that redundant as the item is already type T&& ?
• L. F.: @Hex You have to still use move because item is an lvalue although it is of type T&&. That said, you can unify the copy push and move push a pass-by-value push (see point 2 and 3).
• L. F.: @Hex Yes, exactly. The pass-by-value push shown in point 2 and 3 handles both copy and move properly.