Re: [PATCH] git: remove unneeded casts

From: Jeff King <peff@peff.net>
Date: Dec 17, 2022, 13:55 UTC
Message-ID: <Y53KNbX7SO+FOoKf@coredump.intra.peff.net>
In-Reply-To: <221215.86edt12bte.gmgdl@evledraar.gmail.com>

On Thu, Dec 15, 2022 at 11:07:24AM +0100, Ævar Arnfjörð Bjarmason wrote:

> Excluding compat/ the diff is the below. Some of it's broken, but some
> of it suggests things that are worth picking up, e.g. the return value
> of xmalloc() being cast (a C++-ism), "(char *)NULL" (do we ever need to
> cast NULL?)

Yeah, getting rid of explicit casts to/from void (as in xmalloc()) is a good thing, IMHO.

Casting NULL is trickier. In normal assignment, no, it should never be needed. When passed to a variadic function (which covers all the cases in your patch below), the compiler needs to know that it's a pointer, and not the integer 0. So of the two allowed null pointer constants, only one is guaranteed to work:

  execl(foo, 0); /* bad! there's nothing to say it's a pointer */
  execl(foo, (char *)0); /* ok */

though whether it matters in practice depends on your ABI, I think.

But what about NULL? My copy of C99 (7.17.3) says it "expands to an implementation-defined null pointer constant". So it could be either of those.

And that is backed up by looking at "git log -S')NULL'", which yields 5d314759d7 (Cast execl*() NULL sentinels to (char *), 2010-07-24).

That said, I think we have lots of bare NULLs passed to variadic functions. Any function with LAST_ARG_MUST_BE_NULL will have a NULL in each of its callers, and we do not bother casting most of them. So I think this is one of those "technically could violate the standard, but OK in practice" things.

-Peff