Skip to content
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

r64 iterator: allow moving from beyond the start / end #578

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

SLieve
Copy link
Contributor

@SLieve SLieve commented Jan 29, 2024

Fixes #574.

Previously calling roaring64_iterator_previous after roaring64_iterator_advance returned false would not move the iterator. With this commit, this behavior and its inverse are both possible.

I chose to implement this by adding a new field in the iterator to indicate saturation. Another option would be to allow the ART iterator to allow moving from beyond the start / end, but that would require changing the index to a signed integer and thus take up more space.

Previously calling `roaring64_iterator_previous` after
`roaring64_iterator_advance` returned false would not move the iterator. With
this commit, this behavior and its inverse are both possible.

I chose to implement this by adding a new field in the iterator to indicate
saturation. Another option would be to allow the ART iterator to allow moving
from beyond the start / end, but that would require changing the index to a
signed integer and thus take up more space.
@SLieve SLieve marked this pull request as ready for review January 31, 2024 08:15
@SLieve SLieve requested a review from Dr-Emann January 31, 2024 08:15
@SLieve SLieve merged commit a550733 into RoaringBitmap:master Feb 1, 2024
20 checks passed
@SLieve SLieve deleted the r64_edge branch February 1, 2024 17:42
SLieve added a commit to SLieve/CRoaring that referenced this pull request Feb 3, 2024
…#578)

Previously calling `roaring64_iterator_previous` after
`roaring64_iterator_advance` returned false would not move the iterator. With
this commit, this behavior and its inverse are both possible.

I chose to implement this by adding a new field in the iterator to indicate
saturation. Another option would be to allow the ART iterator to allow moving
from beyond the start / end, but that would require changing the index to a
signed integer and thus take up more space.
@Dr-Emann
Copy link
Member

Dr-Emann commented Feb 5, 2024

@SLieve I believe with this implementation, it is safe to call roaring64_iterator_advance after the iterator has already reached the end (and vice-versa) unlike what the documentation says:

* Once this returns false, `roaring64_iterator_advance` should not be called on
* the iterator again. Calling `roaring64_iterator_previous` is allowed.

Is this something we want to document, or is it better to leave it open to allow us to make it unsafe in the future? (Personally, in the rust bindings, I'm currently keeping track of "which end did we last go over" as well, and it would be nice to avoid having to keep that extra state if we're willing to guarantee it's safe to do so.)

@SLieve
Copy link
Contributor Author

SLieve commented Feb 6, 2024

For consistency across the API I would lean toward disallowing it (as we disallow it in the 32-bit version), but I don't feel strongly. Alternatively we can also make it safe in the 32-bit version, which sounds nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Roaring64 iterator cannot come back from the edge
2 participants