From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sun, 21 Sep 2025 13:06:26 GMT
Subject: Re: [PATCH v3 05/10] xdiff: delete struct diffdata_t
Message-ID: <7979d8f5-1e97-4429-891a-678adb3ba54f@gmail.com>
In-Reply-To: <807ce3e5aae0253dd58c7a4d0bd04ca341b070c8.1758294992.git.gitgitgadget@gmail.com>

Hi Ezekiel

On 19/09/2025 16:16, Ezekiel Newren via GitGitGadget wrote:
> From: Ezekiel Newren <ezekielnewren@gmail.com>
> 
> Every field in this struct is an alias for a certain field in xdfile_t.
> 
> diffdata_t.nrec   -> xdfile_t.nreff
> diffdata_t.ha     -> xdfile_t.ha
> diffdata_t.rindex -> xdfile_t.rindex
> diffdata_t.rchg   -> xdfile_t.rchg

That explains some of the changes here (so long as one assumes the 
aliasing is a bad thing) but it does not explain why it is a good idea 
to remove the local variables rchg[12] and rindex[12] and instead 
dereference xdf[12] inside the loops

Thanks

Phillip

> Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
> ---
>   xdiff/xdiffi.c | 32 ++++++++------------------------
>   xdiff/xdiffi.h | 11 ++---------
>   2 files changed, 10 insertions(+), 33 deletions(-)
> 
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index 5a96e36dfb..bbf0161f84 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -257,10 +257,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,
>    * sub-boxes by calling the box splitting function. Note that the real job
>    * (marking changed lines) is done in the two boundary reaching checks.
>    */
> -int xdl_recs_cmp(diffdata_t *dd1, long off1, long lim1,
> -		 diffdata_t *dd2, long off2, long lim2,
> +int xdl_recs_cmp(xdfile_t *xdf1, long off1, long lim1,
> +		 xdfile_t *xdf2, long off2, long lim2,
>   		 long *kvdf, long *kvdb, int need_min, xdalgoenv_t *xenv) {
> -	unsigned long const *ha1 = dd1->ha, *ha2 = dd2->ha;
> +	unsigned long const *ha1 = xdf1->ha, *ha2 = xdf2->ha;
>   
>   	/*
>   	 * Shrink the box by walking through each diagonal snake (SW and NE).
> @@ -273,17 +273,11 @@ int xdl_recs_cmp(diffdata_t *dd1, long off1, long lim1,
>   	 * be obviously changed.
>   	 */
>   	if (off1 == lim1) {
> -		char *rchg2 = dd2->rchg;
> -		long *rindex2 = dd2->rindex;
> -
>   		for (; off2 < lim2; off2++)
> -			rchg2[rindex2[off2]] = 1;
> +			xdf2->rchg[xdf2->rindex[off2]] = 1;
>   	} else if (off2 == lim2) {
> -		char *rchg1 = dd1->rchg;
> -		long *rindex1 = dd1->rindex;
> -
>   		for (; off1 < lim1; off1++)
> -			rchg1[rindex1[off1]] = 1;
> +			xdf1->rchg[xdf1->rindex[off1]] = 1;
>   	} else {
>   		xdpsplit_t spl;
>   		spl.i1 = spl.i2 = 0;
> @@ -300,9 +294,9 @@ int xdl_recs_cmp(diffdata_t *dd1, long off1, long lim1,
>   		/*
>   		 * ... et Impera.
>   		 */
> -		if (xdl_recs_cmp(dd1, off1, spl.i1, dd2, off2, spl.i2,
> +		if (xdl_recs_cmp(xdf1, off1, spl.i1, xdf2, off2, spl.i2,
>   				 kvdf, kvdb, spl.min_lo, xenv) < 0 ||
> -		    xdl_recs_cmp(dd1, spl.i1, lim1, dd2, spl.i2, lim2,
> +		    xdl_recs_cmp(xdf1, spl.i1, lim1, xdf2, spl.i2, lim2,
>   				 kvdf, kvdb, spl.min_hi, xenv) < 0) {
>   
>   			return -1;
> @@ -318,7 +312,6 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>   	long ndiags;
>   	long *kvd, *kvdf, *kvdb;
>   	xdalgoenv_t xenv;
> -	diffdata_t dd1, dd2;
>   	int res;
>   
>   	if (xdl_prepare_env(mf1, mf2, xpp, xe) < 0)
> @@ -357,16 +350,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>   	xenv.snake_cnt = XDL_SNAKE_CNT;
>   	xenv.heur_min = XDL_HEUR_MIN_COST;
>   
> -	dd1.nrec = xe->xdf1.nreff;
> -	dd1.ha = xe->xdf1.ha;
> -	dd1.rchg = xe->xdf1.rchg;
> -	dd1.rindex = xe->xdf1.rindex;
> -	dd2.nrec = xe->xdf2.nreff;
> -	dd2.ha = xe->xdf2.ha;
> -	dd2.rchg = xe->xdf2.rchg;
> -	dd2.rindex = xe->xdf2.rindex;
> -
> -	res = xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,
> +	res = xdl_recs_cmp(&xe->xdf1, 0, xe->xdf1.nreff, &xe->xdf2, 0, xe->xdf2.nreff,
>   			   kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0,
>   			   &xenv);
>   	xdl_free(kvd);
> diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h
> index 126c9d8ff4..49e52c67f9 100644
> --- a/xdiff/xdiffi.h
> +++ b/xdiff/xdiffi.h
> @@ -24,13 +24,6 @@
>   #define XDIFFI_H
>   
>   
> -typedef struct s_diffdata {
> -	long nrec;
> -	unsigned long const *ha;
> -	long *rindex;
> -	char *rchg;
> -} diffdata_t;
> -
>   typedef struct s_xdalgoenv {
>   	long mxcost;
>   	long snake_cnt;
> @@ -46,8 +39,8 @@ typedef struct s_xdchange {
>   
>   
>   
> -int xdl_recs_cmp(diffdata_t *dd1, long off1, long lim1,
> -		 diffdata_t *dd2, long off2, long lim2,
> +int xdl_recs_cmp(xdfile_t *xdf1, long off1, long lim1,
> +		 xdfile_t *xdf2, long off2, long lim2,
>   		 long *kvdf, long *kvdb, int need_min, xdalgoenv_t *xenv);
>   int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>   		xdfenv_t *xe);