-
Notifications
You must be signed in to change notification settings - Fork 0
Enhance ring buffer implementation with additional features and tests #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
002c14f
cdcb97c
4423210
8461f1d
4f3a415
cd69180
c72bd51
40a89ad
cf5f003
0b9d7d7
55aec70
235c9ce
879256e
da18e64
32b99a4
27807fd
d49855f
cc02225
4a8f2ef
b068e63
c52050a
e6cbd21
d06bf03
bd7f23a
2e75f29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,12 @@ | ||
| .idea | ||
| cmake-build-debug | ||
| cmake-build-release | ||
| build | ||
| Testing/Temporary | ||
| _codeql_build_dir | ||
| _codeql_detected_source_root | ||
|
|
||
| # CodeQL build artifacts | ||
| _codeql_build_dir | ||
| _codeql_detected_source_root | ||
|
|
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |||||||||||||||||||||||||||||||||||
| #include <type_traits> | ||||||||||||||||||||||||||||||||||||
| #include <algorithm> | ||||||||||||||||||||||||||||||||||||
| #include <cstring> | ||||||||||||||||||||||||||||||||||||
| #include <utility> | ||||||||||||||||||||||||||||||||||||
| #include <vector> | ||||||||||||||||||||||||||||||||||||
| #pragma once | ||||||||||||||||||||||||||||||||||||
| namespace buffers { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -62,12 +63,16 @@ namespace buffers { | |||||||||||||||||||||||||||||||||||
| [[nodiscard]] const_pointer operator->() const noexcept { | ||||||||||||||||||||||||||||||||||||
| return &((*source_)[index_]); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| [[nodiscard]] self_type& operator++() noexcept { | ||||||||||||||||||||||||||||||||||||
| index_ = ++index_ % N; | ||||||||||||||||||||||||||||||||||||
| self_type& operator++() noexcept { | ||||||||||||||||||||||||||||||||||||
| ++count_; | ||||||||||||||||||||||||||||||||||||
| if (count_ >= source_->size()) { | ||||||||||||||||||||||||||||||||||||
| index_ = N; // Set to sentinel value (out of valid range [0, N-1]) when reaching end | ||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||
| index_ = (index_ + 1) % N; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return *this; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| [[nodiscard]] self_type operator++(int) noexcept { | ||||||||||||||||||||||||||||||||||||
| self_type operator++(int) noexcept { | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
| self_type operator++(int) noexcept { | |
| [[nodiscard]] self_type operator++(int) noexcept { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍 / 👎.
Copilot
AI
Jan 18, 2026
There was a problem hiding this comment.
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
AI
Jan 18, 2026
There was a problem hiding this comment.
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.
| 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])); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix exception-safety in move_impl for non‑trivial types.
If a move construction throws, the current cleanup can destroy uninitialized storage because size_ is preset to rhs.size_. Track constructed count and destroy only those, then set size_ after success.
🐛 Proposed fix: track constructed elements and set size_ after success
- void move_impl(self_type& rhs, std::false_type) {
- tail_ = rhs.tail_;
- head_ = rhs.head_;
- size_ = rhs.size_;
+ void move_impl(self_type& rhs, std::false_type) {
+ tail_ = rhs.tail_;
+ size_ = 0;
`#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_;
- }
- throw;
- }
+ size_type constructed = 0;
+ try {
+ for (; constructed < rhs.size_; ++constructed) {
+ new(elements_ + ((tail_ + constructed) % N))
+ T(std::move(rhs[(tail_ + constructed) % N]));
+ }
+ } catch (...) {
+ while (constructed > 0) {
+ --constructed;
+ destroy((tail_ + constructed) % N,
+ bool_constant<is_trivially_destructible_v<value_type>>{});
+ }
+ head_ = tail_;
+ size_ = 0;
+ throw;
+ }
+ size_ = rhs.size_;
`#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])));
- if (!p) {
- break;
- }
- }
- if (!p) {
- while(!empty()) {
- destroy(tail_, bool_constant<is_trivially_destructible_v<value_type>>{});
- tail_ = ++tail_ % N;
- --size_;
- }
- }
+ size_type constructed = 0;
+ for (; constructed < rhs.size_; ++constructed) {
+ new(elements_ + ((tail_ + constructed) % N))
+ T(std::move(rhs[(tail_ + constructed) % N]));
+ }
+ size_ = constructed;
`#endif`
+ head_ = (tail_ + size_) % N;
rhs.clear();
}🤖 Prompt for AI Agents
In `@RingBuffer.hpp` around lines 295 - 335, In move_impl(self_type& rhs,
std::false_type) you must not set size_ to rhs.size_ before element
move-construction because a thrown exception will cause destroy(...) to run on
uninitialized slots; instead track how many elements you actually constructed
(e.g., constructed_count), leave size_ at 0 during construction, for-loop
constructing elements_ + ((tail_ + i) % N) from rhs and increment
constructed_count, on catch/detected failure destroy only the constructed_count
items (using destroy(tail_ + offset, ...)), adjust tail_/head_ as needed while
decrementing constructed_count, rethrow the exception (or handle failure path),
and only after all moves succeed set size_ = rhs.size_ and finally rhs.clear();
apply the same constructed-count pattern in both the __cpp_exceptions and the
no-exceptions branch and reference elements_, tail_, head_, size_, destroy,
storage_type and rhs.clear() when making the changes.
Copilot
AI
Jan 18, 2026
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swap_impl for non-trivial types lacks exception safety.
This implementation uses three move_impl calls through a temporary. If move_impl throws during the second or third move:
- Line 345: If this throws,
*thisis already cleared by line 344, andrhsis partially moved. Both buffers may be in inconsistent states. - Line 346: If this throws,
*thishasrhs's old data, butrhsis cleared andtempstill holds*this's original data.
The strong exception guarantee cannot be provided with this approach when moves can throw. Consider documenting that swap provides only basic exception guarantee, or implement a more careful rollback mechanism.
🤖 Prompt for AI Agents
In `@RingBuffer.hpp` around lines 342 - 347, The swap_impl(self_type& rhs,
std::false_type) implementation is not exception-safe when move_impl can throw;
if the second or third move_impl throws the two buffers can be left in
inconsistent states. Fix by either documenting that swap_impl only offers the
basic exception guarantee or implement a rollback: perform moves into
temporaries (use a local temp for *this and another temp for rhs), then commit
by moving temporaries into targets only after all moves succeed, or catch
exceptions and restore original state by moving back from any succeeded
temporaries; update references to swap_impl, move_impl and
self_type/std::false_type accordingly so the logic uses fail-safe temporaries
and a clear commit/rollback path.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The [[nodiscard]] attribute was removed from the prefix increment operator. This attribute is useful for preventing accidental use as a statement (e.g., ++it; when the return value is important). While prefix increment is commonly used for its side effect, having [[nodiscard]] can help catch bugs where the user meant to use the result. Consider keeping this attribute for consistency with modern C++ best practices.