On Wed, Mar 20, 2019 at 5:20 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Wed, Mar 20, 2019 at 2:10 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> > I'm now pretty satisfied with this. Barring objections, I'll commit this
> > in the next few days. Please review, if you have a chance.
>
> You're defining SIMPLE8B_MAX_VALUE but never use it. Maybe you wanted
> to add an assert / explicit test in intset_add_member()?
>
> /*
> * We buffer insertions in a simple array, before packing and inserting them
> * into the B-tree. MAX_BUFFERED_VALUES sets the size of the buffer. The
> * encoder assumes that it is large enough, that we can always fill a leaf
> * item with buffered new items. In other words, MAX_BUFFERED_VALUES must be
> * larger than MAX_VALUES_PER_LEAF_ITEM.
> */
> #define MAX_BUFFERED_VALUES (MAX_VALUES_PER_LEAF_ITEM * 2)
>
> The *2 is not immediately obvious here (at least it wasn't to me),
> maybe explaining intset_flush_buffered_values() main loop rationale
> here could be worthwhile.
>
> Otherwise, everything looks just fine!
I forgot to mention a minor gripe about the intset_binsrch_uint64 /
intset_binsrch_leaf function, which are 99% duplicates. But I don't
know if fixing that (something like passing the array as a void * and
passing a getter function?) is worth the trouble.
