From: Ezekiel Newren Date: Fri, 14 Nov 2025 06:02:16 GMT Subject: Re: [PATCH v3 04/10] xdiff: use size_t for xrecord_t.size Message-ID: In-Reply-To: On Tue, Nov 11, 2025 at 4:08 PM Junio C Hamano wrote: > > "Ezekiel Newren via GitGitGadget" writes: > > > From: Ezekiel Newren > > > > size_t is the appropriate type because size is describing the number of > > elements, bytes in this case, in memory. > > > > Signed-off-by: Ezekiel Newren > > --- > > xdiff/xdiffi.c | 7 +++---- > > xdiff/xemit.c | 8 ++++---- > > xdiff/xmerge.c | 16 ++++++++-------- > > xdiff/xprepare.c | 6 +++--- > > xdiff/xtypes.h | 2 +- > > 5 files changed, 19 insertions(+), 20 deletions(-) > > This step looks mostly OK but it is messy in some places. > > > diff --git a/xdiff/xemit.c b/xdiff/xemit.c > > index ead930088a..2f8007753c 100644 > > --- a/xdiff/xemit.c > > +++ b/xdiff/xemit.c > > @@ -27,7 +27,7 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t * > > { > > xrecord_t *rec = &xdf->recs[ri]; > > > > - if (xdl_emit_diffrec((char const *)rec->ptr, rec->size, pre, strlen(pre), ecb) < 0) > > + if (xdl_emit_diffrec((char const *)rec->ptr, (long)rec->size, pre, strlen(pre), ecb) < 0) > > On platforms where long is narrower than size_t, we'd tentatively > leave things broken until we update xdl_emit_diffrec() to take > size_t, as it would become too noisy to change it in the same patch, > I guess? > > > @@ -113,8 +113,8 @@ static long match_func_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri, > > xrecord_t *rec = &xdf->recs[ri]; > > > > if (!xecfg->find_func) > > - return def_ff((const char *)rec->ptr, rec->size, buf, sz); > > - return xecfg->find_func((const char *)rec->ptr, rec->size, buf, sz, xecfg->find_func_priv); > > + return def_ff((const char *)rec->ptr, (long)rec->size, buf, sz); > > + return xecfg->find_func((const char *)rec->ptr, (long)rec->size, buf, sz, xecfg->find_func_priv); > > Ditto. > > > diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c > > index 75cb3e76a2..0dd4558a32 100644 > > --- a/xdiff/xmerge.c > > +++ b/xdiff/xmerge.c > > @@ -101,8 +101,8 @@ static int xdl_merge_cmp_lines(xdfenv_t *xe1, int i1, xdfenv_t *xe2, int i2, > > xrecord_t *rec2 = xe2->xdf2.recs + i2; > > > > for (i = 0; i < line_count; i++) { > > - int result = xdl_recmatch((const char *)rec1[i].ptr, rec1[i].size, > > - (const char *)rec2[i].ptr, rec2[i].size, flags); > > + int result = xdl_recmatch((const char *)rec1[i].ptr, (long)rec1[i].size, > > + (const char *)rec2[i].ptr, (long)rec2[i].size, flags); > > Ditto. > > > @@ -119,11 +119,11 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int nee > > if (count < 1) > > return 0; > > > > - for (i = 0; i < count; size += recs[i++].size) > > + for (i = 0; i < count; size += (int)recs[i++].size) > > if (dest) > > memcpy(dest + size, recs[i].ptr, recs[i].size); > > if (add_nl) { > > - i = recs[count - 1].size; > > + i = (int)recs[count - 1].size; > > if (i == 0 || recs[count - 1].ptr[i - 1] != '\n') { > > if (needs_cr) { > > if (dest) > > This is messier than I expected. Before the precontext of this > hunk, "i" and "count" are both incoming parameters of type "int", so > the same "what if size_t is wider?" puzzlement applies here. At > least, the reason why "i" and "count" is "int" is not because they > want to be able to express negative values, so it shouldn't involve > too much hassle if we later want to change them to size_t to lose > these casts. > > > @@ -324,8 +324,8 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1, > > > > static int recmatch(xrecord_t *rec1, xrecord_t *rec2, unsigned long flags) > > { > > - return xdl_recmatch((const char *)rec1->ptr, rec1->size, > > - (const char *)rec2->ptr, rec2->size, flags); > > + return xdl_recmatch((const char *)rec1->ptr, (long)rec1->size, > > + (const char *)rec2->ptr, (long)rec2->size, flags); > > } > > Same "long may not be wide enough, in which case we'd need further > fixes" applies here. > > > @@ -441,7 +441,7 @@ static int lines_contain_alnum(xdfenv_t *xe, int i, int chg) > > { > > for (; chg; chg--, i++) > > if (line_contains_alnum((const char *)xe->xdf2.recs[i].ptr, > > - xe->xdf2.recs[i].size)) > > + (long)xe->xdf2.recs[i].size)) > > return 1; > > return 0; > > } > > Ditto. > > > diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c > > index 4c56467076..b3219aed3e 100644 > > --- a/xdiff/xprepare.c > > +++ b/xdiff/xprepare.c > > @@ -99,8 +99,8 @@ static int xdl_classify_record(unsigned int pass, xdlclassifier_t *cf, xrecord_t > > hi = (long) XDL_HASHLONG(rec->ha, cf->hbits); > > for (rcrec = cf->rchash[hi]; rcrec; rcrec = rcrec->next) > > if (rcrec->rec.ha == rec->ha && > > - xdl_recmatch((const char *)rcrec->rec.ptr, rcrec->rec.size, > > - (const char *)rec->ptr, rec->size, cf->flags)) > > + xdl_recmatch((const char *)rcrec->rec.ptr, (long)rcrec->rec.size, > > + (const char *)rec->ptr, (long)rec->size, cf->flags)) > > Ditto. mmbuffer_t holds all of the bytes of the file in memory, so the number of lines referenced in mmbuffer_t has to be less than or equal to that, which makes the point about long vs size_t moot for this patch series. Maybe int vs size_t is a different story, but there are many other places that use `int` that limit the number of lines in a file that aren't touched at all in this patch series. I will update these types, but in a future patch series because they cause a refactor avalanche in many places. I don't like the current state that Xdiff is in either. That's why I intend to keep going with my xdiff cleanup series. > > @@ -157,7 +157,7 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_ > > goto abort; > > crec = &xdf->recs[xdf->nrec++]; > > crec->ptr = (uint8_t const *)prev; > > - crec->size = (long) (cur - prev); > > + crec->size = cur - prev; > > Yay! > > > diff --git a/xdiff/xtypes.h b/xdiff/xtypes.h > > index b1c520a378..88b1fe4649 100644 > > --- a/xdiff/xtypes.h > > +++ b/xdiff/xtypes.h > > @@ -40,7 +40,7 @@ typedef struct s_chastore { > > > > typedef struct s_xrecord { > > uint8_t const *ptr; > > - long size; > > + size_t size; > > Yay, too! > > > unsigned long ha; > > } xrecord_t; I agree. It's nice to see some clean code in this patch series.