On Thu, Dec 15, 2022 at 08:49:45AM +0900, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > I'll stop here for now. It's a fair bit of leg-work digging
> > these up (though again, I do think there's value in
> > understanding why the cast was there, even if we know it
> > isn't _currently_ doing anything).
>
> I agree with the value of understanding why each of these casts has
> become unnecessary, and thanks for demonstrating how a rerolled
> version should justify its changes with its findings behind each of
> the unnecessary casts.
I almost didn't write my earlier email, because I was worried that I was
creating work where it didn't need to exist (and all of the cases I
looked at seemed like good changes). But the cases that Phillip found
are a good reminder of the value of being careful here.
> What do you recommend the next round should look like? Multi patch
> series, each of which removes one cast with its proposed log message
> explains how it has become unneeded? A single patch with a gigantic
> proposed log message that lists the findings for each and all casts
> that are removed? Somewhere in between, perhaps split along the
> file boundary, or grouped by the event that made them unneeded
> (e.g. "cmd_main() used to take non-const but when we made it to take
> const, all of these casts we remove in this patch became unneeded")?
When I've done fixes across the code base like this in the past, I'd
usually try to group them into patches by root cause. So perhaps:
- this used to be "char **argv" because of main(), but either because
of becoming a builtin or cmd_main() it is now const
- this used to require casting from non-const to const to free()
- etc, with one-offs for cases that don't fit in any group
The keeps a single patch's explanation from being too overwhelming, and
avoids repeating yourself when the same logic applies to multiple cases.
That said, I am happy with any solution that explains why we are sure
each case is OK to do. :)
-Peff
