From: Phillip Wood <phillip.wood123@gmail.com>
To: Rose via GitGitGadget <gitgitgadget@gmail.com>, git@vger.kernel.org
Cc: Rose <83477269+AtariDreams@users.noreply.github.com>,
Seija Kijin <doremylover123@gmail.com>, Jeff King <peff@peff.net>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] git: remove unneeded casts
Date: Thu, 15 Dec 2022 10:18:29 +0000 [thread overview]
Message-ID: <5352a684-8d47-ea1d-ee87-92e4722d47ce@dunelm.org.uk> (raw)
In-Reply-To: <pull.1396.git.git.1671032126602.gitgitgadget@gmail.com>
On 14/12/2022 15:35, Rose via GitGitGadget wrote:
> 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.
> 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
> 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.
> 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
prev parent reply other threads:[~2022-12-15 10:18 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-14 15:35 [PATCH] git: remove unneeded casts Rose via GitGitGadget
2022-12-14 21:16 ` Jeff King
2022-12-14 23:49 ` Junio C Hamano
2022-12-17 13:32 ` Jeff King
2022-12-19 1:07 ` Junio C Hamano
2022-12-15 10:07 ` Ævar Arnfjörð Bjarmason
2022-12-17 13:55 ` Jeff King
2022-12-15 10:18 ` Phillip Wood [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5352a684-8d47-ea1d-ee87-92e4722d47ce@dunelm.org.uk \
--to=phillip.wood123@gmail.com \
--cc=83477269+AtariDreams@users.noreply.github.com \
--cc=doremylover123@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).