From: Jeff King <peff@peff.net>
Date: Fri, 10 Oct 2025 06:14:49 GMT
Subject: Re: [PATCH 21/49] builtin/repack.c: factor our "generated_pack_install"
Message-ID: <20251010061449.GH1965904@coredump.intra.peff.net>
In-Reply-To: <aOV3cuZhab7xOdez@nand.local>

On Tue, Oct 07, 2025 at 04:26:26PM -0400, Taylor Blau wrote:

> > > +	for_each_string_list_item(item, &names)
> > > +		generated_pack_install((struct generated_pack *)item->util,
> >
> > This cast should be unnecessary, right? `item->util` is a void pointer,
> > so C should do the cast implicitly.
> 
> It's unnecessary, but I dislike implicit casts from 'void*' to any other
> type. This makes it clearer how we're supposed to interpret the value in
> item->util, but I'm happy to change it to use the implicit cast if you
> feel strongly about it.

I tend to avoid casts when possible, because they can mask unexpected
conversions. E.g., if item->util's type changed, your cast means we
would never notice it.

But that cuts both ways. If the prototype for generated_pack_install()
changes, it is only your cast that would tell the compiler that this
caller needs to be updated. Ultimately it is the spot that _sets_
item->util that needs to change, but hopefully flagging this spot would
point us in the right direction.

And that does seem more likely than item->util changing away from void.
(To be clear, I don't think either is that likely, I'm just musing on
whether casts like this are helpful in general).

-Peff