Subject: Re: [PATCH v5 13/13] xdiff: change type of xdfile_t.changed from char to bool

From: Phillip Wood <[email-masked]>
Date: Wed, 24 Sep 2025 11:21:46 +0100

On 23/09/2025 22:24, Ezekiel Newren via GitGitGadget wrote:
> From: Ezekiel Newren <[email-masked]>
> 
> The only values possible for 'changed' is 1 and 0, which exactly maps
> to a bool type. It might not look like this is the case because
> matches1 and matches2 (which use to be dis1, and dis2) were also char
> and were assigned numerical values within a few lines of 'changed'
> (what used to be rchg).
> 
> Using NONE, SOME, TOO_MANY for matches1[i]/matches2[j], and true/false
> for changed[k] makes it clear to future readers that these are
> logically separate concepts.

Nicely explained - I think this change is a very good idea and 
separating it out like this makes it much clearer what's going on 
compared to V4.

Thanks

Phillip

> Best-viewed-with: --color-words
> Signed-off-by: Ezekiel Newren <[email-masked]>
> ---
>   xdiff/xdiffi.c     | 12 ++++++------
>   xdiff/xhistogram.c |  8 ++++----
>   xdiff/xpatience.c  |  8 ++++----
>   xdiff/xprepare.c   | 12 ++++++------
>   xdiff/xtypes.h     |  2 +-
>   5 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index 5535452061..b902be9d0e 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->changed[xdf2->rindex[off2]] = 1;
> +			xdf2->changed[xdf2->rindex[off2]] = true;
>   	} else if (off2 == lim2) {
>   		for (; off1 < lim1; off1++)
> -			xdf1->changed[xdf1->rindex[off1]] = 1;
> +			xdf1->changed[xdf1->rindex[off1]] = true;
>   	} else {
>   		xdpsplit_t spl;
>   		spl.i1 = spl.i2 = 0;
> @@ -753,8 +753,8 @@ 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->changed[g->start++] = 0;
> -		xdf->changed[g->end++] = 1;
> +		xdf->changed[g->start++] = false;
> +		xdf->changed[g->end++] = true;
>   
>   		while (xdf->changed[g->end])
>   			g->end++;
> @@ -774,8 +774,8 @@ 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->changed[--g->start] = 1;
> -		xdf->changed[--g->end] = 0;
> +		xdf->changed[--g->start] = true;
> +		xdf->changed[--g->end] = false;
>   
>   		while (xdf->changed[g->start - 1])
>   			g->start--;
> diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c
> index 15ca15f6b0..6dc450b1fe 100644
> --- a/xdiff/xhistogram.c
> +++ b/xdiff/xhistogram.c
> @@ -318,11 +318,11 @@ redo:
>   
>   	if (!count1) {
>   		while(count2--)
> -			env->xdf2.changed[line2++ - 1] = 1;
> +			env->xdf2.changed[line2++ - 1] = true;
>   		return 0;
>   	} else if (!count2) {
>   		while(count1--)
> -			env->xdf1.changed[line1++ - 1] = 1;
> +			env->xdf1.changed[line1++ - 1] = true;
>   		return 0;
>   	}
>   
> @@ -335,9 +335,9 @@ redo:
>   	else {
>   		if (lcs.begin1 == 0 && lcs.begin2 == 0) {
>   			while (count1--)
> -				env->xdf1.changed[line1++ - 1] = 1;
> +				env->xdf1.changed[line1++ - 1] = true;
>   			while (count2--)
> -				env->xdf2.changed[line2++ - 1] = 1;
> +				env->xdf2.changed[line2++ - 1] = true;
>   			result = 0;
>   		} else {
>   			result = histogram_diff(xpp, env,
> diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c
> index 14092ffb86..669b653580 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.changed[line2++ - 1] = 1;
> +			env->xdf2.changed[line2++ - 1] = true;
>   		return 0;
>   	} else if (!count2) {
>   		while(count1--)
> -			env->xdf1.changed[line1++ - 1] = 1;
> +			env->xdf1.changed[line1++ - 1] = true;
>   		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.changed[line1++ - 1] = 1;
> +			env->xdf1.changed[line1++ - 1] = true;
>   		while(count2--)
> -			env->xdf2.changed[line2++ - 1] = 1;
> +			env->xdf2.changed[line2++ - 1] = true;
>   		xdl_free(map.entries);
>   		return 0;
>   	}
> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
> index e1d575f779..070d220f3b 100644
> --- a/xdiff/xprepare.c
> +++ b/xdiff/xprepare.c
> @@ -273,7 +273,7 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
>   
>   	/*
>   	 * Create temporary arrays that will help us decide if
> -	 * changed[i] should remain 0 or become 1.
> +	 * changed[i] should remain false, or become true.
>   	 */
>   	if (!XDL_CALLOC_ARRAY(matches1, xdf1->nrec + 1)) {
>   		status = -1;
> @@ -305,16 +305,16 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
>   
>   	/*
>   	 * Use temporary arrays to decide if changed[i] should remain
> -	 * 0 or become 1.
> +	 * false, or become true.
>   	 */
>   	for (nreff = 0, i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart];
>   	     i <= xdf1->dend; i++, recs++) {
>   		if (matches1[i] == SOME ||
>   		    (matches1[i] == TOO_MANY && !xdl_clean_mmatch(matches1, i, xdf1->dstart, xdf1->dend))) {
>   			xdf1->rindex[nreff++] = i;
> -			/* changed[i] remains 0 */
> +			/* changed[i] remains false */
>   		} else
> -			xdf1->changed[i] = 1;
> +			xdf1->changed[i] = true;
>   	}
>   	xdf1->nreff = nreff;
>   
> @@ -323,9 +323,9 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
>   		if (matches2[i] == SOME ||
>   		    (matches2[i] == TOO_MANY && !xdl_clean_mmatch(matches2, i, xdf2->dstart, xdf2->dend))) {
>   			xdf2->rindex[nreff++] = i;
> -			/* changed[i] remains 0 */
> +			/* changed[i] remains false */
>   		} else
> -			xdf2->changed[i] = 1;
> +			xdf2->changed[i] = true;
>   	}
>   	xdf2->nreff = nreff;
>   
> diff --git a/xdiff/xtypes.h b/xdiff/xtypes.h
> index c4b5d2d8fa..f145abba3e 100644
> --- a/xdiff/xtypes.h
> +++ b/xdiff/xtypes.h
> @@ -48,7 +48,7 @@ typedef struct s_xdfile {
>   	xrecord_t *recs;
>   	long nrec;
>   	long dstart, dend;
> -	char *changed;
> +	bool *changed;
>   	long *rindex;
>   	long nreff;
>   } xdfile_t;