Skip to content

Buffer index when deleting an item #232

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.

Sign up for GitHub

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

Jump to bottom
Closed
priandsf opened this issue Aug 20, 2019 · 2 comments
Closed

Buffer index when deleting an item #232

priandsf opened this issue Aug 20, 2019 · 2 comments

Comments

@priandsf
Copy link
Contributor

priandsf commented Aug 20, 2019 •
edited
Loading

buffer.remove() resets its first index when it gets empty with the code bellow:

      if(!buffer.length) {
        buffer.first = 1;
        buffer.next = 1;
      }

This actually has a few issues:

  • It resets it to 1 while it should better use startIndex. This has consequences when the start index is 0. Even better, it should use minIndex if it exists.
  • It does not reset eof. Suppose that we start from the following buffer {first=10, length=5, eof=true}. Now we remove the 5 elements in the buffer. It leads to a situation where first=1 but eof=true. As a result, it thinks that 1 is the latest element and thus it does not display the remaining ones up to 10.

I suggest the following fix:

      if(!buffer.length) {
        buffer.first = buffer.minIndex!==undefined ? buffer.minIndex : startIndex;
        buffer.next = buffer.first;
        buffer.eof = buffer.eof && buffer.first==buffer.maxIndex;
      }
@dhilt dhilt mentioned this issue Aug 28, 2019
Merged
@dhilt
Copy link
Member

dhilt commented Aug 28, 2019

@priandsf I opened PR #233. There a lot of changes, but they are mostly refactoring. Three files relate to our problem:

adapter.js
buffer.js
BufferCleanupSpec.js

The spec file contains scenarios, you may see how the indexes are evolving during cleaning the Buffer with and without immutableTop option in BOF/EOF and non-BOF/EOF cases.

Looks like it works without changes you suggested above. Moreover, my buffer.js change is also unnecessary. It is here for a consistency purpose that doesn't impact the behaviour.

May I ask you to look at this and say what do you think? does it satisfy your needs? do you have some scenarios which would be broken with this approach? can you introduce these scenarios in BufferCleanupSpec?

@dhilt
Copy link
Member

dhilt commented Oct 2, 2020

Closing per v1.8.2 containing the fix is released.

@dhilt dhilt closed this as completed Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dhilt @priandsf