From: Junio C Hamano Date: Tue, 11 Nov 2025 22:53:09 GMT Subject: Re: [PATCH v3 03/10] xdiff: make xrecord_t.ptr a uint8_t instead of char Message-ID: In-Reply-To: <83e7bf180a380a625c9dc324333ba4a46a4c17c1.1762890152.git.gitgitgadget@gmail.com> "Ezekiel Newren via GitGitGadget" writes: > From: Ezekiel Newren > > Make xrecord_t.ptr uint8_t because it's referring to bytes in memory. > > Every usage of this field was inspected and cast to char*, or similar, "inspected and changed to cast to"? > to avoid signedness warnings/errors from the compiler. Casting was used > so that the whole of xdiff doesn't need to be refactored in order to > change the type of this field. > Signed-off-by: Ezekiel Newren > --- > xdiff/xdiffi.c | 8 ++++---- > xdiff/xemit.c | 6 +++--- > xdiff/xmerge.c | 14 +++++++------- > xdiff/xpatience.c | 2 +- > xdiff/xprepare.c | 6 +++--- > xdiff/xtypes.h | 2 +- > xdiff/xutils.c | 4 ++-- > 7 files changed, 21 insertions(+), 21 deletions(-) > > diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c > index 6f3998ee54..411a8aa69f 100644 > --- a/xdiff/xdiffi.c > +++ b/xdiff/xdiffi.c > @@ -407,7 +407,7 @@ static int get_indent(xrecord_t *rec) > int ret = 0; > > for (i = 0; i < rec->size; i++) { > - char c = rec->ptr[i]; > + uint8_t c = rec->ptr[i]; rec->ptr[] is now an array of uint8_t, so this is not "inspected and cast to". It is unclear from limited context lines how 'c' is used by the existing code, but one example here ... > if (!XDL_ISSPACE(c)) > return ret; ... in the post context assumes that XDL_ISSPACE(), which was designed to work with `char` (of implementation-defined signedness), would safely accept an `unsigned char` (let's admit it; for all practical purposes, uint8_t is equivalent to unsigned char while we are looking at C code) so the updated code should work fine. The definition of XDL_ISSPACE(c) indeed casts `c` to "unsigned char" as the first thing it does, and other tests in this if/else if cascade (hidden in the post context of this hunk) are equality comparisons with ' ' and '\t', so this conversion is safe. Either way would work so it is a minor point, but instead of changing type of `c` to u8 than casting it to `char`, as the proposed log message explained, i.e., char c = (char)rec->ptr[i]; would have been much easier to reason about why this code after the patch is still correct. > @@ -382,10 +382,10 @@ static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m, > * we have a very simple mmfile structure. > */ > t1.ptr = (char *)xe1->xdf2.recs[m->i1].ptr; > - t1.size = xe1->xdf2.recs[m->i1 + m->chg1 - 1].ptr > + t1.size = (char *)xe1->xdf2.recs[m->i1 + m->chg1 - 1].ptr > + xe1->xdf2.recs[m->i1 + m->chg1 - 1].size - t1.ptr; The ptr member in the t1 and t2 struct is still of type (char *), so the size computation is performed as ptrdiff between two (char *), which makes sense. > @@ -156,7 +156,7 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_ > if (XDL_ALLOC_GROW(xdf->recs, xdf->nrec + 1, narec)) > goto abort; > crec = &xdf->recs[xdf->nrec++]; > - crec->ptr = prev; > + crec->ptr = (uint8_t const *)prev; > crec->size = (long) (cur - prev); Hmm, it is tempting to fix this while at it, but I guess the ".size" member being "long" will be updated to use ptrdiff_t or something more appropriate in a later step. Looking sensible.