Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
002c14f
Enhance ring buffer implementation with additional features and tests
bugparty Jan 18, 2026
cdcb97c
Initial plan
Copilot Jan 18, 2026
4423210
Initial plan
Copilot Jan 18, 2026
8461f1d
Initial plan
Copilot Jan 18, 2026
4f3a415
Initial plan
Copilot Jan 18, 2026
cd69180
Add tests for move and swap operations with non-trivially copyable types
Copilot Jan 18, 2026
c72bd51
Enhance iterator equality tests based on review feedback
Copilot Jan 18, 2026
40a89ad
Final status update
Copilot Jan 18, 2026
cf5f003
Remove codeql build artifacts and update .gitignore
Copilot Jan 18, 2026
0b9d7d7
Finalize iterator equality test enhancements
Copilot Jan 18, 2026
55aec70
Update .gitignore to exclude CodeQL build directory
Copilot Jan 18, 2026
235c9ce
Merge pull request #4 from bugparty/copilot/sub-pr-2-again
bugparty Jan 18, 2026
879256e
Fix iterator equality operators to compare only source and index
Copilot Jan 18, 2026
da18e64
Fix iterator equality to compare only source and index
Copilot Jan 18, 2026
32b99a4
Improve code clarity based on code review
Copilot Jan 18, 2026
27807fd
Changes before error encountered
Copilot Jan 18, 2026
d49855f
Changes before error encountered
Copilot Jan 18, 2026
cc02225
Remove CodeQL build artifacts and update .gitignore
Copilot Jan 18, 2026
4a8f2ef
Merge pull request #6 from bugparty/copilot/sub-pr-2-yet-again
bugparty Jan 18, 2026
b068e63
Merge branch 'improvement' into copilot/sub-pr-2-another-one
bugparty Jan 18, 2026
c52050a
Remove CodeQL build artifacts and update .gitignore
Copilot Jan 18, 2026
e6cbd21
Merge pull request #5 from bugparty/copilot/sub-pr-2-another-one
bugparty Jan 18, 2026
d06bf03
Merge branch 'improvement' into copilot/sub-pr-2
bugparty Jan 18, 2026
bd7f23a
Remove redundant modulo operations and fix end() iterator
Copilot Jan 18, 2026
2e75f29
Merge pull request #3 from bugparty/copilot/sub-pr-2
bugparty Jan 19, 2026
c31b7cc
Document preconditions for element accessors
bugparty May 3, 2026
a7c245e
Fix undefined behavior in C++14 from unsequenced modifications
bugparty May 3, 2026
54c8d3b
Update tests and CMake configuration for RingBuffer
bugparty May 3, 2026
ac4043d
Add documentation for ring buffer implementation and clarify iterator…
bugparty May 3, 2026
c1aaaba
Add [[nodiscard]] to iterator increment operators
bugparty May 3, 2026
dd67dd0
Add test demonstrating swap lacks strong exception safety guarantee
bugparty May 3, 2026
ee52493
remove null pointer checks in the move_impl function because in place…
bugparty May 3, 2026
64b5492
refactor: reimplement swap_impl and update related exception tests
bugparty May 3, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
.idea
cmake-build-debug
cmake-build-release
build
Testing/Temporary

32 changes: 18 additions & 14 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
cmake_minimum_required(VERSION 3.21)
project(RingBufferTest)
project(RingBuffer LANGUAGES CXX)

set(Simulate_Android_ToolChain OFF)
if (Simulate_Android_ToolChain)
Expand All @@ -9,19 +9,23 @@ else()
set(CMAKE_CXX_STANDARD 17)
endif()

# Add Google Test
# Include FetchContent to download and manage external dependencies
include(FetchContent)
FetchContent_Declare(
googletest
URL https://github.com/google/googletest/archive/refs/tags/release-1.11.0.zip
)
FetchContent_MakeAvailable(googletest)
add_library(RingBuffer INTERFACE)
add_library(RingBuffer::RingBuffer ALIAS RingBuffer)
target_include_directories(RingBuffer INTERFACE ${CMAKE_CURRENT_SOURCE_DIR})

enable_testing()
add_executable(RingBufferTest test_main.cpp)
target_link_libraries(RingBufferTest gtest gtest_main)
option(RINGBUFFER_BUILD_TESTS "Build RingBuffer tests" ON)
if (RINGBUFFER_BUILD_TESTS)
include(FetchContent)
FetchContent_Declare(
googletest
URL https://github.com/google/googletest/archive/refs/tags/release-1.11.0.zip
)
FetchContent_MakeAvailable(googletest)

enable_testing()
add_executable(RingBufferTest test_main.cpp)
target_link_libraries(RingBufferTest gtest gtest_main RingBuffer::RingBuffer)

include(GoogleTest)
gtest_discover_tests(RingBufferTest)
include(GoogleTest)
gtest_discover_tests(RingBufferTest)
endif()
37 changes: 35 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ This project demonstrates many modern C++ best practices:
### Build Instructions

```bash
git clone https://github.com/yourusername/RingBuffer.git
cd RingBuffer
git clone https://github.com/bugparty/RingBufferCpp.git
cd RingBufferCpp
mkdir build && cd build
cmake ..
cmake --build .
Expand All @@ -60,6 +60,39 @@ This will:

---

## FetchContent Integration

```cmake
include(FetchContent)
FetchContent_Declare(
RingBuffer
GIT_REPOSITORY https://github.com/bugparty/RingBufferCpp.git
GIT_TAG main
)
FetchContent_MakeAvailable(RingBuffer)

target_link_libraries(your_target PRIVATE RingBuffer::RingBuffer)
```

To skip building tests in your parent project, set:

```cmake
set(RINGBUFFER_BUILD_TESTS OFF)
```

---

## Testing Policy

When changing the library, run the tests before pushing changes:

```bash
cmake --build build
ctest --output-on-failure --test-dir build
```

---

Usage

```cpp
Expand Down
112 changes: 107 additions & 5 deletions RingBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <type_traits>
#include <algorithm>
#include <cstring>
#include <utility>
#include <vector>
#pragma once
namespace buffers {
Expand Down Expand Up @@ -62,12 +63,12 @@ namespace buffers {
[[nodiscard]] const_pointer operator->() const noexcept {
return &((*source_)[index_]);
}
[[nodiscard]] self_type& operator++() noexcept {
self_type& operator++() noexcept {
Comment thread
bugparty marked this conversation as resolved.
Outdated
index_ = ++index_ % N;
++count_;
return *this;
}
[[nodiscard]] self_type operator++(int) noexcept {
self_type operator++(int) noexcept {
Comment thread
bugparty marked this conversation as resolved.
Outdated
auto result = *this;
++(*this);
return result;
Expand All @@ -78,6 +79,9 @@ namespace buffers {
[[nodiscard]] size_type count() const noexcept {
return count_;
}
[[nodiscard]] buffer_t source() const noexcept {
return source_;
}
~ring_buffer_iterator() = default;
private:
buffer_t source_{};
Expand All @@ -88,13 +92,13 @@ namespace buffers {
template<typename T, size_t N, bool C, bool Overwrite>
bool operator==(ring_buffer_iterator<T,N,C,Overwrite> const& l,
ring_buffer_iterator<T,N,C,Overwrite> const& r) noexcept {
return l.count() == r.count();
return l.source() == r.source() && l.count() == r.count() && l.index() == r.index();
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iterator equality comparison now requires all three fields (source, count, and index) to match. However, for correct iterator semantics, two iterators should be equal if they refer to the same position in the same container, regardless of their count value. The count field tracks how many times the iterator has been incremented and is not relevant for equality. This could cause issues where logically equivalent iterators (same buffer, same position) are incorrectly considered unequal if they were created through different paths. Consider comparing only source and index.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

}

template<typename T, size_t N, bool C, bool Overwrite>
bool operator!=(ring_buffer_iterator<T,N,C,Overwrite> const& l,
ring_buffer_iterator<T,N,C,Overwrite> const& r) noexcept {
return l.count() != r.count();
return l.source() != r.source() || l.count() != r.count() || l.index() != r.index();
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the equality operator, the inequality operator should be based on De Morgan's law applied to the correct equality comparison. If equality should only compare source and index (not count), then inequality should be: l.source() != r.source() || l.index() != r.index()

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

}

}
Expand Down Expand Up @@ -134,11 +138,14 @@ using std::bool_constant;
using iterator = detail::ring_buffer_iterator<T, N, false, Overwrite>;
using const_iterator = detail::ring_buffer_iterator<T, N, true, Overwrite>;

// Create an empty ring buffer.
ring_buffer() noexcept = default;
// Copy contents and state from another buffer.
ring_buffer(ring_buffer const& rhs) noexcept(is_nothrow_copy_constructible_v<value_type>)
{
copy_impl(rhs, bool_constant<is_trivially_copyable_v<T>>{});
}
// Assign from another buffer.
ring_buffer& operator=(ring_buffer const& rhs) noexcept(is_nothrow_copy_constructible_v<value_type>) {
if(this == &rhs)
return *this;
Expand All @@ -148,10 +155,31 @@ using std::bool_constant;

return *this;
}
// Move contents and state from another buffer.
ring_buffer(ring_buffer&& rhs) noexcept(std::is_nothrow_move_constructible<value_type>::value)
{
move_impl(rhs, bool_constant<is_trivially_copyable_v<T>>{});
}
// Move-assign from another buffer.
ring_buffer& operator=(ring_buffer&& rhs) noexcept(std::is_nothrow_move_constructible<value_type>::value) {
if(this == &rhs)
return *this;

destroy_all(bool_constant<is_trivially_copyable_v<T>>{});
move_impl(rhs, bool_constant<is_trivially_copyable_v<T>>{});

return *this;
}
// Swap contents with another buffer.
void swap(self_type& rhs) noexcept(noexcept(swap_impl(rhs, bool_constant<is_trivially_copyable_v<T>>{}))) {
swap_impl(rhs, bool_constant<is_trivially_copyable_v<T>>{});
}
// Append an element, overwriting when configured.
template<typename U>
void push_back(U&& value) {
push_back(std::forward<U>(value), bool_constant<Overwrite>{});
}
// Remove the oldest element if present.
void pop_front() noexcept{
if(empty())
return;
Expand All @@ -161,36 +189,52 @@ using std::bool_constant;
--size_;
tail_ = ++tail_ %N;
}
// Access the newest element.
[[nodiscard]] reference back() noexcept {
return reinterpret_cast<reference>(elements_[(head_ + N - 1) % N]);
}
// Access the newest element (const).
[[nodiscard]] const_reference back() const noexcept {
return const_cast<self_type*>(this)->back();
}
// Access the oldest element.
[[nodiscard]] reference front() noexcept { return reinterpret_cast<reference >(elements_[tail_]); }
// Access the oldest element (const).
[[nodiscard]] const_reference front() const noexcept {
return const_cast<self_type*>(this)->front();
}
// Direct access by internal storage index.
[[nodiscard]] reference operator[](size_type index) noexcept {
return reinterpret_cast<reference >(elements_[index]);
}
// Direct access by internal storage index (const).
[[nodiscard]] const_reference operator[](size_type index) const noexcept {
return const_cast<self_type *>(this)->operator[](index);
}
// Iterator to oldest element.
[[nodiscard]] iterator begin() noexcept { return iterator{this, tail_, 0};}
// Iterator to one past newest element.
[[nodiscard]] iterator end() noexcept { return iterator{this, head_, size_};}
// Const iterator to oldest element.
[[nodiscard]] const_iterator cbegin() const noexcept { return const_iterator{this, tail_, 0};}
// Const iterator to one past newest element.
[[nodiscard]] const_iterator cend() const noexcept { return const_iterator{this, head_, size_};}
// Check if buffer has no elements.
[[nodiscard]] bool empty() const noexcept { return size_ == 0; }
// Check if buffer is at capacity.
[[nodiscard]] bool full() const noexcept { return size_ == N; }
// Current element count.
[[nodiscard]] size_type size() const noexcept { return size_; }
// Maximum element count.
[[nodiscard]] size_type capacity() const noexcept { return N; }
// Remove all elements and reset indices.
void clear() noexcept {
destroy_all(bool_constant<is_trivially_destructible_v<value_type>>{});
size_ = 0;
head_ = 0;
tail_ = 0;
}
// Destroy elements on teardown.
~ring_buffer() {
clear();
};
Expand All @@ -204,7 +248,7 @@ using std::bool_constant;
}
}
void copy_impl(self_type const& rhs, std::true_type) {
std::memcpy(elements_, rhs.elements_, rhs.size_ * sizeof(T));
std::memcpy(elements_, rhs.elements_, N * sizeof(T));
size_ = rhs.size_;
tail_ = rhs.tail_;
head_ = rhs.head_;
Expand Down Expand Up @@ -248,6 +292,59 @@ using std::bool_constant;

#endif
}
void move_impl(self_type& rhs, std::true_type) {
std::memcpy(elements_, rhs.elements_, N * sizeof(T));
size_ = rhs.size_;
tail_ = rhs.tail_;
head_ = rhs.head_;
rhs.clear();
}
void move_impl(self_type& rhs, std::false_type) {
tail_ = rhs.tail_;
head_ = rhs.head_;
size_ = rhs.size_;
#ifdef __cpp_exceptions
try {
for (auto i = 0; i < size_; ++i)
new( elements_ + ((tail_ + i) % N)) T(std::move(rhs[(tail_ + i) % N]));
}catch(...) {
while(!empty()) {
destroy(tail_, bool_constant<std::is_trivially_destructible_v<value_type>>{});
tail_ = ++tail_ % N;
--size_;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Track constructed elements when move throws

If T's move constructor throws during move_impl (non-trivial path), the catch block destroys elements in a loop driven by size_, which was set to rhs.size_ before construction. When the throw happens partway through, only some slots were constructed, so this loop can call destroy on uninitialized storage, which is undefined behavior and can crash for types with throwing moves. This can surface in move construction, move assignment, and swap_impl for such types. Consider tracking the number of successfully constructed elements (or updating size_ incrementally) and only destroying those.

Useful? React with 👍 / 👎.

}
throw;
}
#else
storage_type *p = nullptr;
for (auto i = 0; i < size_; ++i) {
p =reinterpret_cast<storage_type *>(new(elements_ + ((tail_ + i) % N)) T(std::move(rhs[(tail_ + i) % N])));
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after equals sign. Should be 'p = reinterpret_cast' instead of 'p =reinterpret_cast' for consistency with code formatting standards.

Copilot uses AI. Check for mistakes.
if (!p) {
break;
}
}
if (!p) {
while(!empty()) {
destroy(tail_, bool_constant<is_trivially_destructible_v<value_type>>{});
tail_ = ++tail_ % N;
--size_;
}
}
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The placement new expression will never return nullptr. Placement new either successfully constructs the object or throws an exception (if exceptions are enabled). The check 'if (!p)' will never be true because placement new always returns the pointer passed to it. This error handling logic is ineffective and should be removed.

Suggested change
storage_type *p = nullptr;
for (auto i = 0; i < size_; ++i) {
p =reinterpret_cast<storage_type *>(new(elements_ + ((tail_ + i) % N)) T(std::move(rhs[(tail_ + i) % N])));
if (!p) {
break;
}
}
if (!p) {
while(!empty()) {
destroy(tail_, bool_constant<is_trivially_destructible_v<value_type>>{});
tail_ = ++tail_ % N;
--size_;
}
}
for (auto i = 0; i < size_; ++i) {
new(elements_ + ((tail_ + i) % N)) T(std::move(rhs[(tail_ + i) % N]));
}

Copilot uses AI. Check for mistakes.
#endif
rhs.clear();
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
void swap_impl(self_type& rhs, std::true_type) noexcept {
std::swap(elements_, rhs.elements_);
std::swap(head_, rhs.head_);
std::swap(tail_, rhs.tail_);
std::swap(size_, rhs.size_);
}
Comment on lines +338 to +343
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The swap_impl for trivially copyable types attempts to swap C-style arrays (elements_), which cannot be done with std::swap. C-style arrays are not assignable. This implementation will not compile. The correct approach is to swap element-by-element or use std::swap_ranges for the arrays.

Copilot uses AI. Check for mistakes.
void swap_impl(self_type& rhs, std::false_type) {
self_type temp;
temp.move_impl(*this, std::false_type{});
this->move_impl(rhs, std::false_type{});
rhs.move_impl(temp, std::false_type{});
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
template<typename U>
void push_back(U&& value, std::true_type) {
push_back_impl(std::forward<U>(value));
Expand Down Expand Up @@ -284,5 +381,10 @@ using std::bool_constant;
size_type size_{};
};

template<typename T, size_t N, bool Overwrite>
void swap(ring_buffer<T, N, Overwrite>& lhs, ring_buffer<T, N, Overwrite>& rhs) noexcept(noexcept(lhs.swap(rhs))) {
lhs.swap(rhs);
}

}
#endif //RINGBUFFERTEST_RINGBUFFER_HPP
Loading