From: Ezekiel Newren <ezekielnewren@gmail.com>
Date: Wed, 24 Sep 2025 14:46:31 GMT
Subject: Re: [PATCH v5 12/13] xdiff: use enum macros NONE(0), SOME(1), TOO_MANY(2) in xprepare.c
Message-ID: <CAH=ZcbBcEbTqQ-FpYSdr_QjSM5sKerECKsSVdTyHoSQqG-V9iA@mail.gmail.com>
In-Reply-To: <311f7dbe-b970-4a5b-9d53-05f019cc54cd@gmail.com>

On Wed, Sep 24, 2025 at 4:21 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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]"

"Rename dis1, dis2 to matches1, matches2 to give the variable names a
more obvious meaning."

Would something like that work, or do I need to refine it further? I
would love to rename rdis, rpdis, etc... except that I don't
understand what is happening or why. Could someone explain the purpose
of these variables?

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

What is the reflow limit for comments? 72? 80?

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

Thanks.

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

I can do that.

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

I didn't add !! and thought it looked funny myself. I didn't remove it
because I wasn't sure if I should.