Re: [PATCH] git-compat-util.h: introduce CALLOC(x)

From: Taylor Blau <me@ttaylorr.com>
Date: Dec 7, 2022, 02:34 UTC
Message-ID: <Y4/7txDTWK5nGPDu@nand.local>
In-Reply-To: <221206.868rjle7za.gmgdl@evledraar.gmail.com>

On Tue, Dec 06, 2022 at 12:12:32AM +0100, Ævar Arnfjörð Bjarmason wrote:

> First, when you add *.pending.cocci rules they shouldn't be pseudocode,
> but things that are too large to apply right now. I think my recent
> 041df69edd3 (Merge branch 'ab/fewer-the-index-macros', 2022-11-28) is a
> good example.

Agreed, but I do tend to consider this patch too-large to apply right now.

Whether or not 2k lines of diff is review-able or not (and FWIW, I think that your recent Coccinelle-generated patches were quite easy to review pretty quickly), I think there's a bigger question of whether or not we want to make such a sweeping, tree-wide change. And assuming the answer to that is "yes", there's is also a question of timing. Proposing it towards the middle or end of a release cycle seems in bad taste.

But I think splitting this discussion up into, "should we introduce something like CALLOC() into our style conventions?" and "do we want to apply this everywhere?" is worth doing. The former doesn't seem to take a ton of time away from polishing the release candidates, and the latter can be done only after the former has settled.

So the decision to make this a *.pending.cocci rule was definitely intentional in this case.

Show 9 quoted lines

> Second, we have test support for rules now, see
> contrib/coccinelle/tests/. You just need to create a
> contrib/coccinelle/tests/xcalloc.pending.c, and have the expected
> post-image in contrib/coccinelle/tests/xcalloc.pending.res. Please add
> one of those. We don't have them for existing rules, but it really helps
> to assert & review new rules.
>
> The various edge cases that your current *.cocci doesn't compile on
> etc. should go into that test file.

Thanks for mentioning.

> Third, the resulting patch is currently ~2k lines. Can we really not
> make it non-pending with some whitelisting/gblacklisting. E.g. see this
> out-of-tree patch for an example of opting out certain functions:
> https://github.com/avar/git/commit/bedd0323e9b

See above.

Show 13 quoted lines

> Fourth, I must say I'm kind of negative on the proposed change. I.e. the
> foot-gun is definitely worth solving, but why not just create a
> coccinelle rule to narrowly address that? In that case we can presumably
> start with it non-pending, as we don't have that many of them.
>
> On the notion that we need to special-case:
>
> 	- CALLOC_ARRAY(ptr, 1)
> 	+ CALLOC(ptr)
>
> Because an array of one is "not an array" I don't really buy it. The
> calloc() interface itself exposes that, so I don't see why we'd consider
> those separate on the API level for these wrapper macros.

I think the point is that it's just weird. Yes, an array of just a single element on the heap is indistinguishable from asking malloc() to give me the same bytes and then memset() them myself, just as it's indistinguishable from calloc()'ing the right number of bytes for a single structure to begin with.

But whether or not the two are indistinguishable doesn't mean that it makes intuitive sense, and that is the goal of CALLOC().

Thanks, Taylor