From: Ezekiel Newren Date: Thu, 06 Nov 2025 23:20:21 GMT Subject: Re: [PATCH v2 06/10] xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash Message-ID: In-Reply-To: On Thu, Nov 6, 2025 at 4:00 AM Phillip Wood wrote: > > Hi Ezekiel > > On 29/10/2025 22:19, Ezekiel Newren via GitGitGadget wrote: > > From: Ezekiel Newren > > > > The ha field is serving two different purposes, which makes the code > > harder to read. At first glance it looks like many places assume > > there could never be hash collisions between lines of the two input > > files. In reality, line_hash is used together with xdl_recmatch() to > > ensure correct comparisons of lines, even when collisions occur. > > > > To make this clearer, the old ha field has been split: > > * line_hash: The straightforward hash of a line, requiring no > > additional context. > > * minimal_perfect_hash: Not a new concept, but now a separate > > field. It comes from the classifier's general-purpose hash table, > > which assigns each line a unique and minimal hash across the two > > files. > > It would be nice to explain the differing types for the two fields in > the commit message. I'll add something like: line_hash is a uint64_t because it is the output of a fixed width hash function. minimal_perfect_hash is size_t because its purpose is to index into an array. This also avoids the problem of having to cast to usize on the Rust side every time minimal_perfect_hash is used to index a slice. > > diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c > > index 85e56021da..16236bd045 100644 > > --- a/xdiff/xprepare.c > > +++ b/xdiff/xprepare.c > > @@ -96,9 +96,9 @@ static int xdl_classify_record(unsigned int pass, xdlclassifier_t *cf, xrecord_t > > long hi; > > xdlclass_t *rcrec; > > > > - hi = (long) XDL_HASHLONG(rec->ha, cf->hbits); > > + hi = (long) XDL_HASHLONG(rec->line_hash, cf->hbits); > > "hi" is only used as an array index so it might be nicer to change it to > size_t and avoid this cast instead. I agree. I'll make that change.