Re: [PATCH] git: remove unneeded casts

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Dec 15, 2022, 10:18 UTC
Message-ID: <5352a684-8d47-ea1d-ee87-92e4722d47ce@dunelm.org.uk>
In-Reply-To: <pull.1396.git.git.1671032126602.gitgitgadget@gmail.com>

On 14/12/2022 15:35, Rose via GitGitGadget wrote:

Show 5 quoted lines

> From: Seija Kijin <doremylover123@gmail.com>
> 
> Many of these casts remain,
> even though the target variable is the type it is being casted to.
> We can safely remove said casts.

Like Peff I curious how you found these. I also agree that it would be helpful to understand the history so we can be sure there the cast is not a sympton of a bug - Peff has given you a great start on that. A lot of the changes look like useful cleanups but there are a couple that caught my eye as being wrong or undesirable which I've commented on below.

Show 10 quoted lines

> diff --git a/strbuf.c b/strbuf.c
> index 0890b1405c5..940f59473eb 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -493,7 +493,7 @@ void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags)
>   		if (ch <= 0x1F || ch >= 0x7F ||
>   		    (ch == '/' && (flags & STRBUF_ENCODE_SLASH)) ||
>   		    strchr(URL_UNSAFE_CHARS, ch))
> -			strbuf_addf(dst, "%%%02X", (unsigned char)ch);
> +			strbuf_addf(dst, "%%%02X", ch);

This one is wrong, the cast is required if char is signed as in that case it will be converted to a signed integer (because printf() takes a variable number of arguments) and then %X treats that integer as unsigned. That means that if the high bit is set we'll now print a bunch of leading F's. To see this compile and run

#include <stdio.h>

int main(void)
{
         printf("%02X %02X\n", (signed char)0x80, (unsigned char)0x80);
         return 0;
}

which prints

FFFFFF80 80

Show 10 quoted lines

> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
> index c84549f6c50..04fa4e5a01d 100644
> --- a/xdiff/xprepare.c
> +++ b/xdiff/xprepare.c
> @@ -188,7 +188,7 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_
>   			if (!(crec = xdl_cha_alloc(&xdf->rcha)))
>   				goto abort;
>   			crec->ptr = prev;
> -			crec->size = (long) (cur - prev);
> +			crec->size = (cur - prev);

I'm not sure if we want to remove this cast. cur and prev are pointers so (cur - prev) has type ptrdiff_t which maybe wider than long. We can debate whether we should be using a cast to hide any compiler warning here but the cast is not redundant in the same way as others in this patch.

Show 13 quoted lines

>   			crec->ha = hav;
>   			recs[nrec++] = crec;
>   			if (xdl_classify_record(pass, cf, rhash, hbits, crec) < 0)
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 9e36f24875d..853f2260a1d 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -130,7 +130,7 @@ long xdl_guess_lines(mmfile_t *mf, long sample) {
>   			else
>   				cur++;
>   		}
> -		tsize += (long) (cur - data);
> +		tsize += (cur - data);

This is in the same class as the one above. (Also if we do end up removing the cast we should remove the parentheses as well)

Best Wishes

Phillip