[WIP] Update ring buffer enhancement based on review feedback #3
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,23 +49,27 @@ namespace buffers { | |
| ring_buffer_iterator& operator=(ring_buffer_iterator const& ) noexcept = default; | ||
| template<bool Z = C, typename std::enable_if<(!Z), int>::type* = nullptr> | ||
| [[nodiscard]] reference operator*() noexcept { | ||
| return (*source_)[index_]; | ||
| } | ||
| template<bool Z = C, typename std::enable_if<(Z), int>::type* = nullptr> | ||
| [[nodiscard]] const_reference operator*() const noexcept { | ||
| return (*source_)[index_]; | ||
|
There was a problem hiding this comment. Removing the modulo operation from the dereference operator creates a critical bug. When an iterator has index_ set to the sentinel value N (which happens for end() iterators), dereferencing will access (*source_)[N], which is out of bounds. The valid range is [0, N-1]. While dereferencing end() iterators is undefined behavior in standard iterators, the previous implementation would not cause out-of-bounds access. Consider restoring the modulo operation to maintain memory safety, or add bounds checking to prevent accessing index N. |
||
| } | ||
| template<bool Z = C, typename std::enable_if<(!Z), int>::type* = nullptr> | ||
| [[nodiscard]] pointer operator->() noexcept { | ||
| return &((*source_)[index_]); | ||
|
There was a problem hiding this comment. Removing the modulo operation from the arrow operator creates a critical bug. When an iterator has index_ set to the sentinel value N (which happens for end() iterators), dereferencing will access (*source_)[N], which is out of bounds. The valid range is [0, N-1]. While dereferencing end() iterators is undefined behavior in standard iterators, the previous implementation would not cause out-of-bounds access. Consider restoring the modulo operation to maintain memory safety, or add bounds checking to prevent accessing index N. |
||
| } | ||
| template<bool Z = C, typename std::enable_if<(Z), int>::type* = nullptr> | ||
| [[nodiscard]] const_pointer operator->() const noexcept { | ||
| return &((*source_)[index_]); | ||
|
There was a problem hiding this comment. Removing the modulo operation from the arrow operator creates a critical bug. When an iterator has index_ set to the sentinel value N (which happens for end() iterators), dereferencing will access (*source_)[N], which is out of bounds. The valid range is [0, N-1]. While dereferencing end() iterators is undefined behavior in standard iterators, the previous implementation would not cause out-of-bounds access. Consider restoring the modulo operation to maintain memory safety, or add bounds checking to prevent accessing index N. |
||
| } | ||
| self_type& operator++() noexcept { | ||
| ++count_; | ||
| if (count_ >= source_->size()) { | ||
|
Owner
There was a problem hiding this comment. 如果你这样实现，那么上面的 [[nodiscard]] reference operator*() noexcept {
Contributor
Author
There was a problem hiding this comment. You're absolutely right! Fixed in bd7f23a. Since the operator++ keeps index_ in [0, N-1] during valid iteration and only sets it to N when reaching end, the % N in the dereference operators was redundant. I've also updated end() and cend() to consistently use N as the sentinel value. |
||
| index_ = N; // Set to sentinel value (out of valid range [0, N-1]) when reaching end | ||
| } else { | ||
| index_ = (index_ + 1) % N; | ||
| } | ||
| return *this; | ||
| } | ||
| self_type operator++(int) noexcept { | ||
|
|
@@ -214,11 +218,11 @@ using std::bool_constant; | |
| // 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, N, size_};} | ||
|
Comment on lines
219
to
+221
There was a problem hiding this comment.
With end() now returning an iterator at index Useful? React with 👍 / 👎. |
||
| // 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, N, size_};} | ||
| // Check if buffer has no elements. | ||
| [[nodiscard]] bool empty() const noexcept { return size_ == 0; } | ||
| // Check if buffer is at capacity. | ||
|
|
||
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.
Removing the modulo operation from the dereference operator creates a critical bug. When an iterator has index_ set to the sentinel value N (which happens for end() iterators), dereferencing will access (*source_)[N], which is out of bounds. The valid range is [0, N-1].
While dereferencing end() iterators is undefined behavior in standard iterators, the previous implementation would not cause out-of-bounds access. Consider restoring the modulo operation to maintain memory safety, or add bounds checking to prevent accessing index N.