Re: [PATCH v3 03/10] xdiff: make xrecord_t.ptr a uint8_t instead of char
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 11, 2025, 22:53 UTC
- Message-ID
- <xmqqbjl8uqp6.fsf@gitster.g>
- In-Reply-To
- <83e7bf180a380a625c9dc324333ba4a46a4c17c1.1762890152.git.gitgitgadget@gmail.com>
"Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> From: Ezekiel Newren <ezekielnewren@gmail.com> > > 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.
Show 21 quoted lines
> Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
> ---
> 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.
Show 7 quoted lines
> @@ -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.
Show 7 quoted lines
> @@ -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.