From: Phillip Wood Date: Wed, 24 Sep 2025 10:21:46 GMT Subject: Re: [PATCH v5 13/13] xdiff: change type of xdfile_t.changed from char to bool Message-ID: In-Reply-To: <975e845bfa80fe838433f7f64759a72f6d565aea.1758662670.git.gitgitgadget@gmail.com> On 23/09/2025 22:24, Ezekiel Newren via GitGitGadget wrote: > From: Ezekiel Newren > > 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 > --- > 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;