Move in-container iteration into containers.h #546
Conversation
include/roaring/roaring_types.h
Outdated
| uint16_t value; | ||
| bool has_value; |
There was a problem hiding this comment.
It feels unfortunate that roaring_uint32_iterator_t and roaring_container_iterator_t both have value and has_value fields which seem somewhat duplicated, but I'm not sure it would be reasonable to try to change all uses of roaring_uint32_iterator_t to go through functions to get the current value/has_value (combining the container iterator's value and the high bits of the outer iterator)
There was a problem hiding this comment.
In my opinion accessors would be an improvement because it allows us to hide the type and change it in the future without ABI incompatibility. It also makes the fields easier to access in other languages. However, it would be an API change.
There was a problem hiding this comment.
Hiding the type means requiring an allocation and requiring clients to call a cleanup function though, which can be unfortunate. Right now, there's no allocation, and once they're done iterating, they can just return or whatever.
I honestly think it would be a good idea to do it for roaring_bitmap_t (totally unrelated to your changes), but I'm less sold on the iterator.
That said, we could still use accessors (inline?) even if we don't end up hiding the details of the type.
There was a problem hiding this comment.
Fully agreed on roaring_bitmap_t.
Regarding duplication, I've pushed a new commit that removes value and has_value in favor of out parameters. I think it's a bit better, though the function signature of container_iterator_read_into_uint32 is not looking great.
Happy to add accessors to the roaring iterator, though as of right now it won't help reduce the size of the iterator.
SLieve
force-pushed
the
container_it
branch
3 times, most recently
from
3dbce44 to
87c0968
Compare
This adds an iterator to iterate over the entries within a container. Extracting this logic allows reusing it for 64 bit iteration.
This reduces the footprint of the roaring iterator and reduces duplication somewhat.
This PR moves logic for iterating within containers into
containers.h, and adds a newroaring_container_iterator_tstruct for that purpose.roaring_container_iterator_tis defined inroaring_types.hbecause it's exposed throughroaring_uint32_iterator_t. In theory it seems possible to hideroaring_uint32_iterator_t, but since exposed already that would have to be done with consideration.Functionally this should be a no-op. I did make some cosmetic changes and ran clang format on the modified code.
Although it would make the code simpler, I did not include a pointer to the current container and the current typecode in the iterator to avoid duplicating more state. I'm flexible on this though!