Re: [PATCH v5 12/13] xdiff: use enum macros NONE(0), SOME(1), TOO_MANY(2) in xprepare.c

>
> On 23/09/2025 22:24, Ezekiel Newren via GitGitGadget wrote:
> > From: Ezekiel Newren <ezekielnewren@gmail.com>
> >
> > Rename dis1, dis2 to matches1, matches2.
> >
> > Define macros NONE(0), SOME(1), TOO_MANY(2) as the enum values for
> > matches1 and matches2. These states will influence whether changed[i]
> > is set to 1 or kept as 0.
>
> This message also says what is being changed rather than why it is being
> changed. I think the rename here is a good idea but I'm not sure what
> "rdis[01]" and "rpdis[01]" are used for and whether they should be
> renamed if we're renaming "dis[01]"

> >       /*
> > -      * Limits the window the is examined during the similar-lines
> > -      * scan. The loops below stops when dis[i - r] == 1 (line that
> > +      * Limits the window that is examined during the similar-lines
> > +      * scan. The loops below stops when matches[i - r] == SOME (line that
>
> Thanks for updating the comments. Not reflowing the lines makes the diff
> easier to read but leaves the comments in a rather strange state with
> random long lines.

> >        * has no match), but there are corner cases where the loop
> >        * proceed all the way to the extremities by causing huge
> >        * performance penalties in case of big files.
> > @@ -207,40 +210,44 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {
> >
> >       /*
> >        * Scans the lines before 'i' to find a run of lines that either
> > -      * have no match (dis[j] == 0) or have multiple matches (dis[j] > 1).
> > -      * Note that we always call this function with dis[i] > 1, so the
> > +      * have no match (matches[j] == NONE) or have multiple matches (matches[j] == TOO_MANY).
> > +      * Note that we always call this function with matches[i] == TOO_MANY, so the
>
> especially here
>
> > -             if (!dis[i + r])
> > +             if (matches[i + r] == NONE)
> >                       rdis1++;
> > -             else if (dis[i + r] == 2)
> > +             else if (matches[i + r] == TOO_MANY)
> >                       rpdis1++;
> > -             else
> > +             else if (matches[i + r] == SOME)
> >                       break;
> > +             else
> > +                     BUG("Illegal value for matches[i + r]");
>
> Nice addition

> >   static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {
> >       long i, nm, nreff, mlim;
> >       xrecord_t *recs;
> >       xdlclass_t *rcrec;
> > -     char *dis, *dis1, *dis2;
> > -     int need_min = !!(cf->flags & XDF_NEED_MINIMAL);
> > +     uint8_t *matches1, *matches2;
>
> Let's initialize these where they're declared rather than later on

> > +     int status = 0;
> I think we typically we call this "ret" or "res" in the rest of the code
> base.
>
> > +     bool need_min = !!(cf->flags & XDF_NEED_MINIMAL);
>
> Nice use of bool, strictly speaking I don't think we need the !! if
> we're changing the type from int. I think Junio recently suggested that
> we might start using (bool) instead of !! for cases like this.
>
> Everything below this looks good, though some of the lines are getting
> quite long with the renamed variables and symbolic values so we might
> want to break them.