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

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 26, 2022, 00:45 UTC
Message-ID: <Yt85P8p3ePW5WWWc@nand.local>
In-Reply-To: <CAN0heSoA=wv4syJ3VOe92QPpjPHyqUPJ8+Pv+mbB0-TiiieVmw@mail.gmail.com>

Hi Martin,

Show 7 quoted lines

> > > > 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.

Show 18 quoted lines

> > > 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