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
