Re: [PATCH v3 4/6] pack-bitmap: prepare to read lookup table extension
On Mon, Jul 18, 2022 at 2:37 PM Martin Ågren <martin.agren@gmail.com> wrote:
Show 6 quoted lines
>
> Hi Abhradeep and Taylor,
>
> I very much enjoy following from a distance Abhradeep's work on this
> series and all the reviewing and mentoring. I don't grasp anywhere near
> all the details, but I've looked into this a bit:
Show 11 quoted lines
> "The compar routine is expected to have two arguments which point to
> the key object and to an array member, in that order, [...]"
>
> I think it would help to make this something like
>
> static int triplet_cmp(const void *key, const void *array_item)
>
> to really highlight this asymmetric nature of this function, or to make
> clear how the values flow through our call-chain through something like
>
> static int triplet_cmp(const void *commit_pos, const void *table_entry)
> Would it make sense to let the `const void *key` directly carry the
> 32-bit value and hope that `sizeof(key) >= sizeof(uint32_t)`? That's
> probably too magical, "just" to save on dereferencing.
I do not have any particular opinion here. I will do whatever you think is best.
Show 19 quoted lines
> One thing that could perhaps make things clearer is if
> `bsearch_triplet()` did take the position directly, rather than as a
> pointer:
>
> -static int bsearch_triplet(uint32_t *commit_pos,
> +static int bsearch_triplet(uint32_t commit_pos,
> struct bitmap_index *bitmap_git,
> struct bitmap_lookup_table_triplet *triplet)
> {
> - unsigned char *p = bsearch(commit_pos,
> bitmap_git->table_lookup, bitmap_git->entry_count,
> + unsigned char *p = bsearch(&commit_pos,
> bitmap_git->table_lookup, bitmap_git->entry_count,
> BITMAP_LOOKUP_TABLE_TRIPLET_WIDTH,
> triplet_cmp);
>
>
> Also, maybe s/bsearch_triplet/&_by_pos/ could clarify the intent of this
> function?Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>