Subject: Re: [PATCH v3 4/6] pack-bitmap: prepare to read lookup table extension

From: Taylor Blau <[email-masked]>
Date: Mon, 25 Jul 2022 20:45:51 -0400

Hi Martin,

> > > > The comment you added is definitely helpful, but I still think that this
> > > > line is a little magical. `*va` isn't really a pointer to a `uint32_t`,
> > > > but a pointer to the start of a triplet, which just *happens* to have a
> > > > 4-byte integer at the beginning of it.
>
> Yeah, this all looks quite magical with the casting, and with the
> asymmetric handling of `va` and `vb`.

Yeah, this was my main point (which I didn't intend to create as much of
a digression with as I appear to have!).

The handling here is all correct, but what I was saying was that even
though we're treating `*vb` as a pointer to a `uint32_t`, reading vb[1]
is bogus, since there isn't another 32-bit value there.

So I was saying that you *could* initialize a triplet struct, assign its
fields appropriately, and then compare `*va` to `triplet->foo`. But I
think setting up a struct to only bother reading the first field is
probably wasteful, hence my suggestion for a clarifying comment.

> > > Are you sure about this? As far as I know, the first parameter of such
> > > comparing functions is always a pointer to the given key that we need
> > > to search for and the second parameter points to each element of an
> > > array.
>
> Yes, that matches my understanding and the man-page for bsearch(3):
>
>   "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)

Yeah, that makes sense to me. I'm not too attached to either name, both
seem OK to me.

Thanks,
Taylor