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

Fix FixedRingBuffer.removeAt #1572

Merged
merged 1 commit into from Sep 27, 2016

Conversation

Projects
None yet
3 participants
@tchaloupka
Contributor

tchaloupka commented Sep 25, 2016

There is a problem when removing from the start index

Show outdated Hide outdated source/vibe/utils/array.d
@@ -376,7 +376,7 @@ struct FixedRingBuffer(T, size_t N = 0, bool INITIALIZE = true) {
assert(r.m_buffer is m_buffer);
if( m_start + m_fill > m_buffer.length ){
assert(r.m_start >= m_start && r.m_start < m_buffer.length || r.m_start < mod(m_start+m_fill));
if( r.m_start > m_start ){
if( r.m_start >= m_start ){

This comment has been minimized.

@s-ludwig

s-ludwig Sep 25, 2016

Member

Would probably be a nice optimization to do popFront in case of r.m_start == m_start.

@s-ludwig

s-ludwig Sep 25, 2016

Member

Would probably be a nice optimization to do popFront in case of r.m_start == m_start.

This comment has been minimized.

@tchaloupka

tchaloupka Sep 25, 2016

Contributor

Good point, fixed

@tchaloupka

tchaloupka Sep 25, 2016

Contributor

Good point, fixed

This comment has been minimized.

@s-ludwig

s-ludwig Sep 25, 2016

Member

Thanks! Will merge when Travis is done.

@s-ludwig

s-ludwig Sep 25, 2016

Member

Thanks! Will merge when Travis is done.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 25, 2016

Member

Sounds like this might have been responsible for some obscure bugs!

Member

s-ludwig commented Sep 25, 2016

Sounds like this might have been responsible for some obscure bugs!

Show outdated Hide outdated source/vibe/utils/array.d
@@ -376,7 +376,7 @@ struct FixedRingBuffer(T, size_t N = 0, bool INITIALIZE = true) {
assert(r.m_buffer is m_buffer);
if( m_start + m_fill > m_buffer.length ){
assert(r.m_start >= m_start && r.m_start < m_buffer.length || r.m_start < mod(m_start+m_fill));
if( r.m_start > m_start ){
if( r.m_start >= m_start ){

This comment has been minimized.

@s-ludwig

s-ludwig Sep 25, 2016

Member

Thanks! Will merge when Travis is done.

@s-ludwig

s-ludwig Sep 25, 2016

Member

Thanks! Will merge when Travis is done.

Fix FixedRingBuffer.removeAt
There is a problem when removing from the start index
@tchaloupka

This comment has been minimized.

Show comment
Hide comment
@tchaloupka

tchaloupka Sep 25, 2016

Contributor

I just squashed the PR.

Contributor

tchaloupka commented Sep 25, 2016

I just squashed the PR.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 25, 2016

Member

Just discovered that GitHub now also offers this as an option for how to merge the PR, which is pretty nice for avoiding to bother the PR authors with this!

Member

s-ludwig commented Sep 25, 2016

Just discovered that GitHub now also offers this as an option for how to merge the PR, which is pretty nice for avoiding to bother the PR authors with this!

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Sep 25, 2016

Contributor

@s-ludwig note that if you use this GitHub option, merge commit won't be created - instead whole PR content will go as a single commit directly to base branch. This may be or may not be what you want :)

Contributor

mihails-strasuns commented Sep 25, 2016

@s-ludwig note that if you use this GitHub option, merge commit won't be created - instead whole PR content will go as a single commit directly to base branch. This may be or may not be what you want :)

@s-ludwig s-ludwig merged commit 921396b into vibe-d:master Sep 27, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@tchaloupka tchaloupka deleted the tchaloupka:fix_ringBuffer branch Sep 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment