Re: [PATCH 09/17] xdiff: treat xdfile_t.rchg like an enum

From: Elijah Newren <newren@gmail.com>
Date: Sep 9, 2025, 08:58 UTC
Message-ID: <CABPp-BEnvJ2x5sC-fYHHKm7Cqu=CWtNzGBDT10JU-yS4hw2dpQ@mail.gmail.com>
In-Reply-To: <d1657f51012c8ee97f81bf5e3550aa47a0c5b70c.1757274320.git.gitgitgadget@gmail.com>

On Sun, Sep 7, 2025 at 12:46 PM Ezekiel Newren via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 54 quoted lines

>
> From: Ezekiel Newren <ezekielnewren@gmail.com>
>
> Define macros NO(0), YES(1), MAYBE(2) as the enum values for rchg to
> make the code easier to follow. Perhaps 'rchg' should be renamed to
> 'changed'?
>
> Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
> ---
>  xdiff/xdiff.h      |  4 ++++
>  xdiff/xdiffi.c     | 29 ++++++++++++++---------------
>  xdiff/xhistogram.c |  8 ++++----
>  xdiff/xpatience.c  |  8 ++++----
>  xdiff/xprepare.c   | 24 ++++++++++++------------
>  5 files changed, 38 insertions(+), 35 deletions(-)
>
> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
> index 2cecde5afe..7092879829 100644
> --- a/xdiff/xdiff.h
> +++ b/xdiff/xdiff.h
> @@ -27,6 +27,10 @@
>  extern "C" {
>  #endif /* #ifdef __cplusplus */
>
> +#define NO 0
> +#define YES 1
> +#define MAYBE 2
> +
>  /* xpparm_t.flags */
>  #define XDF_NEED_MINIMAL (1 << 0)
>
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index a66125d44a..44fd27823a 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -278,10 +278,10 @@ int xdl_recs_cmp(xdfile_t *xdf1, long off1, long lim1,
>          */
>         if (off1 == lim1) {
>                 for (; off2 < lim2; off2++)
> -                       xdf2->rchg[xdf2->rindex[off2]] = 1;
> +                       xdf2->rchg[xdf2->rindex[off2]] = YES;
>         } else if (off2 == lim2) {
>                 for (; off1 < lim1; off1++)
> -                       xdf1->rchg[xdf1->rindex[off1]] = 1;
> +                       xdf1->rchg[xdf1->rindex[off1]] = YES;
>         } else {
>                 xdpsplit_t spl;
>                 spl.i1 = spl.i2 = 0;
> @@ -708,7 +708,7 @@ struct xdlgroup {
>  static void group_init(xdfile_t *xdf, struct xdlgroup *g)
>  {
>         g->start = g->end = 0;
> -       while (xdf->rchg[g->end])
> +       while (xdf->rchg[g->end] == YES)

You've got a few places like this where the old code would have behaved differently if there were some MAYBE values. I presume you've carefully vetted that those can't happen at these points in the code, but it might be worth calling that out in the commit message for reviewers who'll otherwise wonder if there's a behavior change that has occurred.

Show 9 quoted lines

>                 g->end++;
>  }
>
> @@ -722,7 +722,7 @@ static inline int group_next(xdfile_t *xdf, struct xdlgroup *g)
>                 return -1;
>
>         g->start = g->end + 1;
> -       for (g->end = g->start; xdf->rchg[g->end]; g->end++)
> +       for (g->end = g->start; xdf->rchg[g->end] == YES; g->end++)

Here's another of those where you assume MAYBE isn't possible.

Show 9 quoted lines

>                 ;
>
>         return 0;
> @@ -738,7 +738,7 @@ static inline int group_previous(xdfile_t *xdf, struct xdlgroup *g)
>                 return -1;
>
>         g->end = g->start - 1;
> -       for (g->start = g->end; xdf->rchg[g->start - 1]; g->start--)
> +       for (g->start = g->end; xdf->rchg[g->start - 1] == YES; g->start--)

...and another.

Show 14 quoted lines

>                 ;
>
>         return 0;
> @@ -753,10 +753,10 @@ static int group_slide_down(xdfile_t *xdf, struct xdlgroup *g)
>  {
>         if (g->end < xdf->nrec &&
>             recs_match(&xdf->recs[g->start], &xdf->recs[g->end])) {
> -               xdf->rchg[g->start++] = 0;
> -               xdf->rchg[g->end++] = 1;
> +               xdf->rchg[g->start++] = NO;
> +               xdf->rchg[g->end++] = YES;
>
> -               while (xdf->rchg[g->end])
> +               while (xdf->rchg[g->end] == YES)

...and another.

Show 14 quoted lines

>                         g->end++;
>
>                 return 0;
> @@ -774,10 +774,10 @@ static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g)
>  {
>         if (g->start > 0 &&
>             recs_match(&xdf->recs[g->start - 1], &xdf->recs[g->end - 1])) {
> -               xdf->rchg[--g->start] = 1;
> -               xdf->rchg[--g->end] = 0;
> +               xdf->rchg[--g->start] = YES;
> +               xdf->rchg[--g->end] = NO;
>
> -               while (xdf->rchg[g->start - 1])
> +               while (xdf->rchg[g->start - 1] == YES)

...and another.

Show 20 quoted lines

>                         g->start--;
>
>                 return 0;
> @@ -932,16 +932,15 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>
>  int xdl_build_script(xdfenv_t *xe, xdchange_t **xscr) {
>         xdchange_t *cscr = NULL, *xch;
> -       char *rchg1 = xe->xdf1.rchg, *rchg2 = xe->xdf2.rchg;
>         long i1, i2, l1, l2;
>
>         /*
>          * Trivial. Collects "groups" of changes and creates an edit script.
>          */
>         for (i1 = xe->xdf1.nrec, i2 = xe->xdf2.nrec; i1 >= 0 || i2 >= 0; i1--, i2--)
> -               if (rchg1[i1 - 1] || rchg2[i2 - 1]) {
> -                       for (l1 = i1; rchg1[i1 - 1]; i1--);
> -                       for (l2 = i2; rchg2[i2 - 1]; i2--);
> +               if (xe->xdf1.rchg[i1 - 1] || xe->xdf2.rchg[i2 - 1]) {
> +                       for (l1 = i1; xe->xdf1.rchg[i1 - 1]; i1--);
> +                       for (l2 = i2; xe->xdf2.rchg[i2 - 1]; i2--);

The changes in this xdl_build_script() function appear to be orthogonal to what was described in the commit message. If it's a separate cleanup, perhaps justify it in a separate patch?

Show 136 quoted lines

>
>                         if (!(xch = xdl_add_change(cscr, i1, i2, l1 - i1, l2 - i2))) {
>                                 xdl_free_script(cscr);
> diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c
> index 4d857e8ae2..c2e85b8ab9 100644
> --- a/xdiff/xhistogram.c
> +++ b/xdiff/xhistogram.c
> @@ -318,11 +318,11 @@ redo:
>
>         if (!count1) {
>                 while(count2--)
> -                       env->xdf2.rchg[line2++ - 1] = 1;
> +                       env->xdf2.rchg[line2++ - 1] = YES;
>                 return 0;
>         } else if (!count2) {
>                 while(count1--)
> -                       env->xdf1.rchg[line1++ - 1] = 1;
> +                       env->xdf1.rchg[line1++ - 1] = YES;
>                 return 0;
>         }
>
> @@ -335,9 +335,9 @@ redo:
>         else {
>                 if (lcs.begin1 == 0 && lcs.begin2 == 0) {
>                         while (count1--)
> -                               env->xdf1.rchg[line1++ - 1] = 1;
> +                               env->xdf1.rchg[line1++ - 1] = YES;
>                         while (count2--)
> -                               env->xdf2.rchg[line2++ - 1] = 1;
> +                               env->xdf2.rchg[line2++ - 1] = YES;
>                         result = 0;
>                 } else {
>                         result = histogram_diff(xpp, env,
> diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c
> index bf69a58527..20cda5e258 100644
> --- a/xdiff/xpatience.c
> +++ b/xdiff/xpatience.c
> @@ -331,11 +331,11 @@ static int patience_diff(xpparam_t const *xpp, xdfenv_t *env,
>         /* trivial case: one side is empty */
>         if (!count1) {
>                 while(count2--)
> -                       env->xdf2.rchg[line2++ - 1] = 1;
> +                       env->xdf2.rchg[line2++ - 1] = YES;
>                 return 0;
>         } else if (!count2) {
>                 while(count1--)
> -                       env->xdf1.rchg[line1++ - 1] = 1;
> +                       env->xdf1.rchg[line1++ - 1] = YES;
>                 return 0;
>         }
>
> @@ -347,9 +347,9 @@ static int patience_diff(xpparam_t const *xpp, xdfenv_t *env,
>         /* are there any matching lines at all? */
>         if (!map.has_matches) {
>                 while(count1--)
> -                       env->xdf1.rchg[line1++ - 1] = 1;
> +                       env->xdf1.rchg[line1++ - 1] = YES;
>                 while(count2--)
> -                       env->xdf2.rchg[line2++ - 1] = 1;
> +                       env->xdf2.rchg[line2++ - 1] = YES;
>                 xdl_free(map.entries);
>                 return 0;
>         }
> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
> index 92f9845003..36437f91bb 100644
> --- a/xdiff/xprepare.c
> +++ b/xdiff/xprepare.c
> @@ -215,9 +215,9 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {
>          * current line (i) is already a multimatch line.
>          */
>         for (r = 1, rdis0 = 0, rpdis0 = 1; (i - r) >= s; r++) {
> -               if (!dis[i - r])
> +               if (dis[i - r] == NO)
>                         rdis0++;
> -               else if (dis[i - r] == 2)
> +               else if (dis[i - r] == MAYBE)
>                         rpdis0++;
>                 else
>                         break;
> @@ -231,9 +231,9 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {
>         if (rdis0 == 0)
>                 return 0;
>         for (r = 1, rdis1 = 0, rpdis1 = 1; (i + r) <= e; r++) {
> -               if (!dis[i + r])
> +               if (dis[i + r] == NO)
>                         rdis1++;
> -               else if (dis[i + r] == 2)
> +               else if (dis[i + r] == MAYBE)
>                         rpdis1++;
>                 else
>                         break;
> @@ -273,7 +273,7 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
>         for (i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart]; i <= xdf1->dend; i++, recs++) {
>                 rcrec = cf->rcrecs[recs->ha];
>                 nm = rcrec ? rcrec->len2 : 0;
> -               dis1[i] = (nm == 0) ? 0: (nm >= mlim && !need_min) ? 2: 1;
> +               dis1[i] = (nm == 0) ? NO: (nm >= mlim && !need_min) ? MAYBE: YES;
>         }
>
>         if ((mlim = xdl_bogosqrt(xdf2->nrec)) > XDL_MAX_EQLIMIT)
> @@ -281,26 +281,26 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
>         for (i = xdf2->dstart, recs = &xdf2->recs[xdf2->dstart]; i <= xdf2->dend; i++, recs++) {
>                 rcrec = cf->rcrecs[recs->ha];
>                 nm = rcrec ? rcrec->len1 : 0;
> -               dis2[i] = (nm == 0) ? 0: (nm >= mlim && !need_min) ? 2: 1;
> +               dis2[i] = (nm == 0) ? NO: (nm >= mlim && !need_min) ? MAYBE: YES;
>         }
>
>         for (nreff = 0, i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart];
>              i <= xdf1->dend; i++, recs++) {
> -               if (dis1[i] == 1 ||
> -                   (dis1[i] == 2 && !xdl_clean_mmatch(dis1, i, xdf1->dstart, xdf1->dend))) {
> +               if (dis1[i] == YES ||
> +                   (dis1[i] == MAYBE && !xdl_clean_mmatch(dis1, i, xdf1->dstart, xdf1->dend))) {
>                         xdf1->rindex[nreff++] = i;
>                 } else
> -                       xdf1->rchg[i] = 1;
> +                       xdf1->rchg[i] = YES;
>         }
>         xdf1->nreff = nreff;
>
>         for (nreff = 0, i = xdf2->dstart, recs = &xdf2->recs[xdf2->dstart];
>              i <= xdf2->dend; i++, recs++) {
> -               if (dis2[i] == 1 ||
> -                   (dis2[i] == 2 && !xdl_clean_mmatch(dis2, i, xdf2->dstart, xdf2->dend))) {
> +               if (dis2[i] == YES ||
> +                   (dis2[i] == MAYBE && !xdl_clean_mmatch(dis2, i, xdf2->dstart, xdf2->dend))) {
>                         xdf2->rindex[nreff++] = i;
>                 } else
> -                       xdf2->rchg[i] = 1;
> +                       xdf2->rchg[i] = YES;
>         }
>         xdf2->nreff = nreff;
>
> --
> gitgitgadget

Everything else looks like the straightforward translation you called out in your commit message.