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

[Taylor Blau <me@ttaylorr.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.

> 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.

> 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