Re: [PATCH v3 04/10] xdiff: use size_t for xrecord_t.size

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

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

> From: Ezekiel Newren <ezekielnewren@gmail.com>
>
> 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 <ezekielnewren@gmail.com>
> ---
>  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.

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