Re: [PATCH v3 03/10] xdiff: make xrecord_t.ptr a uint8_t instead of char

[Junio C Hamano <gitster@pobox.com>]

"Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

> 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.

> @@ -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.