From: Elijah Newren <newren@gmail.com>
Date: Tue, 09 Sep 2025 08:56:05 GMT
Subject: Re: [PATCH 02/17] xdiff: delete local variables and initialize/free xdfile_t directly
Message-ID: <CABPp-BHF38TTwzsgJ215GGrz1Za-m1qMX0u4BY7P_UTzOATuaQ@mail.gmail.com>
In-Reply-To: <15832ad2715d377ed9b0c9f3de66cc4532cac2ba.1757274320.git.gitgitgadget@gmail.com>

On Sun, Sep 7, 2025 at 12:48 PM Ezekiel Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Ezekiel Newren <ezekielnewren@gmail.com>
>
> xdl_prepare_ctx() uses local variables and assigns them to the
> corresponding xdfile_t fields if there are no errors. Delete them and
> use the fields of xdfile_t directly.

In particular, those local variables are essentially a hand-rolled
additional implementation of xdl_free_ctx() inlined into
xdl_prepare_ctx().  You're just modifying the code to use the existing
xdl_free_ctx() function so we don't have two ways to free such
variables (especially since one of those two was an ugly inlining).

> Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
> ---
>  xdiff/xprepare.c | 79 +++++++++++++++++++-----------------------------
>  1 file changed, 31 insertions(+), 48 deletions(-)
>
> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
> index a45c5ee208..2ed1785b09 100644
> --- a/xdiff/xprepare.c
> +++ b/xdiff/xprepare.c
> @@ -134,99 +134,82 @@ static int xdl_classify_record(unsigned int pass, xdlclassifier_t *cf, xrecord_t
>  }
>
>
> +static void xdl_free_ctx(xdfile_t *xdf)
> +{
> +

unnecessary blank line here

> +       xdl_free(xdf->rhash);
> +       xdl_free(xdf->rindex);
> +       xdl_free(xdf->rchg - 1);
> +       xdl_free(xdf->ha);
> +       xdl_free(xdf->recs);
> +       xdl_cha_free(&xdf->rcha);
> +}
> +
> +
>  static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_t const *xpp,
>                            xdlclassifier_t *cf, xdfile_t *xdf) {
> -       unsigned int hbits;
> -       long nrec, hsize, bsize;
> +       long bsize;
>         unsigned long hav;
>         char const *blk, *cur, *top, *prev;
>         xrecord_t *crec;
> -       xrecord_t **recs;
> -       xrecord_t **rhash;
> -       unsigned long *ha;
> -       char *rchg;
> -       long *rindex;
>
> -       ha = NULL;
> -       rindex = NULL;
> -       rchg = NULL;
> -       rhash = NULL;
> -       recs = NULL;
> +       xdf->ha = NULL;
> +       xdf->rindex = NULL;
> +       xdf->rchg = NULL;
> +       xdf->rhash = NULL;
> +       xdf->recs = NULL;
>
>         if (xdl_cha_init(&xdf->rcha, sizeof(xrecord_t), narec / 4 + 1) < 0)
>                 goto abort;
> -       if (!XDL_ALLOC_ARRAY(recs, narec))
> +       if (!XDL_ALLOC_ARRAY(xdf->recs, narec))
>                 goto abort;
>
> -       hbits = xdl_hashbits((unsigned int) narec);
> -       hsize = 1 << hbits;
> -       if (!XDL_CALLOC_ARRAY(rhash, hsize))
> +       xdf->hbits = xdl_hashbits((unsigned int) narec);
> +       if (!XDL_CALLOC_ARRAY(xdf->rhash, 1 << xdf->hbits))
>                 goto abort;
>
> -       nrec = 0;
> +       xdf->nrec = 0;
>         if ((cur = blk = xdl_mmfile_first(mf, &bsize))) {
>                 for (top = blk + bsize; cur < top; ) {
>                         prev = cur;
>                         hav = xdl_hash_record(&cur, top, xpp->flags);
> -                       if (XDL_ALLOC_GROW(recs, nrec + 1, narec))
> +                       if (XDL_ALLOC_GROW(xdf->recs, xdf->nrec + 1, narec))
>                                 goto abort;
>                         if (!(crec = xdl_cha_alloc(&xdf->rcha)))
>                                 goto abort;
>                         crec->ptr = prev;
>                         crec->size = (long) (cur - prev);
>                         crec->ha = hav;
> -                       recs[nrec++] = crec;
> -                       if (xdl_classify_record(pass, cf, rhash, hbits, crec) < 0)
> +                       xdf->recs[xdf->nrec++] = crec;
> +                       if (xdl_classify_record(pass, cf, xdf->rhash, xdf->hbits, crec) < 0)
>                                 goto abort;
>                 }
>         }
>
> -       if (!XDL_CALLOC_ARRAY(rchg, nrec + 2))
> +       if (!XDL_CALLOC_ARRAY(xdf->rchg, xdf->nrec + 2))
>                 goto abort;
>
>         if ((XDF_DIFF_ALG(xpp->flags) != XDF_PATIENCE_DIFF) &&
>             (XDF_DIFF_ALG(xpp->flags) != XDF_HISTOGRAM_DIFF)) {
> -               if (!XDL_ALLOC_ARRAY(rindex, nrec + 1))
> +               if (!XDL_ALLOC_ARRAY(xdf->rindex, xdf->nrec + 1))
>                         goto abort;
> -               if (!XDL_ALLOC_ARRAY(ha, nrec + 1))
> +               if (!XDL_ALLOC_ARRAY(xdf->ha, xdf->nrec + 1))
>                         goto abort;
>         }
>
> -       xdf->nrec = nrec;
> -       xdf->recs = recs;
> -       xdf->hbits = hbits;
> -       xdf->rhash = rhash;
> -       xdf->rchg = rchg + 1;
> -       xdf->rindex = rindex;
> +       xdf->rchg += 1;
>         xdf->nreff = 0;
> -       xdf->ha = ha;
>         xdf->dstart = 0;
> -       xdf->dend = nrec - 1;
> +       xdf->dend = xdf->nrec - 1;
>
>         return 0;
>
>  abort:
> -       xdl_free(ha);
> -       xdl_free(rindex);
> -       xdl_free(rchg);
> -       xdl_free(rhash);
> -       xdl_free(recs);
> -       xdl_cha_free(&xdf->rcha);
> +       xdl_free_ctx(xdf);
>         return -1;
>  }
>
>
> -static void xdl_free_ctx(xdfile_t *xdf) {
> -
> -       xdl_free(xdf->rhash);
> -       xdl_free(xdf->rindex);
> -       xdl_free(xdf->rchg - 1);
> -       xdl_free(xdf->ha);
> -       xdl_free(xdf->recs);
> -       xdl_cha_free(&xdf->rcha);
> -}
> -
> -
>  void xdl_free_env(xdfenv_t *xe) {
>
>         xdl_free_ctx(&xe->xdf2);
> --
> gitgitgadget

Looks good.