write a circular array (jazz::buffer::Loop)#16
Conversation
micromho
changed the title
buffer::Loop)jazz::buffer::Loop)
micromho
force-pushed
the
nausicaa/ring-buffer
branch
from
dcdbf38 to
0439756
Compare
| std::array<Sample, Size> buffer_; | ||
| std::ptrdiff_t zero_; | ||
|
|
||
| std::size_t real_index_(std::ptrdiff_t i) const { |
There was a problem hiding this comment.
this seems like a slightly odd function:
- the underscore suffix, for one, makes me think it should just be private
- maybe it should actually just be something that returns a const reference to the backing array, which you can then index into (or iterate over) at your heart's content?
that, or we should just delete it (and not test it)
There was a problem hiding this comment.
added a long comment explaining why it exists and what it does
| * contain the item we just wrote. | ||
| */ | ||
| template <typename Sample, const std::size_t Size> | ||
| class Loop { |
There was a problem hiding this comment.
can you define an ostream& << for this guy, so we can debug-print it?
| Iterator(Loop* loop, std::ptrdiff_t i) : loop_(loop), i_(i) {} | ||
|
|
||
| public: | ||
| // required by std::iterator_traits to know how this thing works |
There was a problem hiding this comment.
probably you want to static_assert the iterator concept to ensure you got it right (and ensure it is not broken in the future)
There was a problem hiding this comment.
added some comments re. why that static_assert is a bit broken, but also added a static_assert somewhere that it's not as broken; see when i pooosh
| Sample Push(Sample s) { | ||
| zero_ = real_index_(-1); | ||
| Sample old = buffer_[zero_]; | ||
| buffer_[zero_] = s; | ||
| return old; | ||
| } | ||
|
|
||
| Sample PushBack(Sample s) { | ||
| Sample old = buffer_[zero_]; | ||
| buffer_[zero_] = s; | ||
| zero_ = real_index_(1); | ||
| return old; | ||
| } |
There was a problem hiding this comment.
the "returns the element we shoved out the other end" behavior of this seems Right, but is perhaps non-intuitive, and so is worth documenting right next to the method imo
|
|
||
| constexpr std::size_t size() const { return Size; } | ||
|
|
||
| Sample Push(Sample s) { |
There was a problem hiding this comment.
should this return std::optional or something? Presumably we don't end up getting an element pushed out if the array was not yet full...
There was a problem hiding this comment.
right now, the array is always full of default-constructed Samples (soon to be Ts), so there's always something to push. i could change this but it fits our current use case so whatevs
There was a problem hiding this comment.
...and in fact, if we want this, we could make T itself std::optional.
Could you, however, document this somewhere?
micromho
force-pushed
the
nausicaa/ring-buffer
branch
from
c346c83 to
47f713e
Compare
2 participants
two considerations:
buffer::CircularBufferor smth for absolute clarity