Re: [PATCH v5 12/13] xdiff: use enum macros NONE(0), SOME(1), TOO_MANY(2) in xprepare.c
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Sep 24, 2025, 10:21 UTC
- Message-ID
- <311f7dbe-b970-4a5b-9d53-05f019cc54cd@gmail.com>
- In-Reply-To
- <08a0fceb72b2bd0a2803d24b9874b7f9bd03703a.1758662670.git.gitgitgadget@gmail.com>
On 23/09/2025 22:24, Ezekiel Newren via GitGitGadget wrote:
Show 7 quoted lines
> From: Ezekiel Newren <ezekielnewren@gmail.com> > > Rename dis1, dis2 to matches1, matches2. > > Define macros NONE(0), SOME(1), TOO_MANY(2) as the enum values for > matches1 and matches2. These states will influence whether changed[i] > is set to 1 or kept as 0.
This message also says what is being changed rather than why it is being changed. I think the rename here is a good idea but I'm not sure what "rdis[01]" and "rpdis[01]" are used for and whether they should be renamed if we're renaming "dis[01]"
Show 5 quoted lines
> /* > - * Limits the window the is examined during the similar-lines > - * scan. The loops below stops when dis[i - r] == 1 (line that > + * Limits the window that is examined during the similar-lines > + * scan. The loops below stops when matches[i - r] == SOME (line that
Thanks for updating the comments. Not reflowing the lines makes the diff easier to read but leaves the comments in a rather strange state with random long lines.
Show 11 quoted lines
> * has no match), but there are corner cases where the loop
> * proceed all the way to the extremities by causing huge
> * performance penalties in case of big files.
> @@ -207,40 +210,44 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {
>
> /*
> * Scans the lines before 'i' to find a run of lines that either
> - * have no match (dis[j] == 0) or have multiple matches (dis[j] > 1).
> - * Note that we always call this function with dis[i] > 1, so the
> + * have no match (matches[j] == NONE) or have multiple matches (matches[j] == TOO_MANY).
> + * Note that we always call this function with matches[i] == TOO_MANY, so theespecially here
Show 11 quoted lines
> - if (!dis[i + r])
> + if (matches[i + r] == NONE)
> rdis1++;
> - else if (dis[i + r] == 2)
> + else if (matches[i + r] == TOO_MANY)
> rpdis1++;
> - else
> + else if (matches[i + r] == SOME)
> break;
> + else
> + BUG("Illegal value for matches[i + r]");Nice addition
Show 7 quoted lines
> static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {
> long i, nm, nreff, mlim;
> xrecord_t *recs;
> xdlclass_t *rcrec;
> - char *dis, *dis1, *dis2;
> - int need_min = !!(cf->flags & XDF_NEED_MINIMAL);
> + uint8_t *matches1, *matches2;Let's initialize these where they're declared rather than later on
> + int status = 0;
I think we typically we call this "ret" or "res" in the rest of the code base.
> + bool need_min = !!(cf->flags & XDF_NEED_MINIMAL);
Nice use of bool, strictly speaking I don't think we need the !! if we're changing the type from int. I think Junio recently suggested that we might start using (bool) instead of !! for cases like this.
Everything below this looks good, though some of the lines are getting quite long with the renamed variables and symbolic values so we might want to break them.
Thanks
Phillip
Show 81 quoted lines
> - if (!XDL_CALLOC_ARRAY(dis, xdf1->nrec + xdf2->nrec + 2))
> - return -1;
> - dis1 = dis;
> - dis2 = dis1 + xdf1->nrec + 1;
> + matches1 = NULL;
> + matches2 = NULL;
> +
> + /*
> + * Create temporary arrays that will help us decide if
> + * changed[i] should remain 0 or become 1.
> + */
> + if (!XDL_CALLOC_ARRAY(matches1, xdf1->nrec + 1)) {
> + status = -1;
> + goto cleanup;
> + }
> + if (!XDL_CALLOC_ARRAY(matches2, xdf2->nrec + 1)) {
> + status = -1;
> + goto cleanup;
> + }
>
> + /*
> + * Initialize temporary arrays with NONE, SOME, or TOO_MANY.
> + */
> if ((mlim = xdl_bogosqrt(xdf1->nrec)) > XDL_MAX_EQLIMIT)
> mlim = XDL_MAX_EQLIMIT;
> 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;
> + matches1[i] = (nm == 0) ? NONE: (nm >= mlim && !need_min) ? TOO_MANY: SOME;
> }
>
> if ((mlim = xdl_bogosqrt(xdf2->nrec)) > XDL_MAX_EQLIMIT)
> @@ -278,14 +300,19 @@ 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;
> + matches2[i] = (nm == 0) ? NONE: (nm >= mlim && !need_min) ? TOO_MANY: SOME;
> }
>
> + /*
> + * Use temporary arrays to decide if changed[i] should remain
> + * 0 or become 1.
> + */
> 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 (matches1[i] == SOME ||
> + (matches1[i] == TOO_MANY && !xdl_clean_mmatch(matches1, i, xdf1->dstart, xdf1->dend))) {
> xdf1->rindex[nreff++] = i;
> + /* changed[i] remains 0 */
> } else
> xdf1->changed[i] = 1;
> }
> @@ -293,17 +320,20 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
>
> 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 (matches2[i] == SOME ||
> + (matches2[i] == TOO_MANY && !xdl_clean_mmatch(matches2, i, xdf2->dstart, xdf2->dend))) {
> xdf2->rindex[nreff++] = i;
> + /* changed[i] remains 0 */
> } else
> xdf2->changed[i] = 1;
> }
> xdf2->nreff = nreff;
>
> - xdl_free(dis);
> +cleanup:
> + xdl_free(matches1);
> + xdl_free(matches2);
>
> - return 0;
> + return status;
> }
>
>