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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Dec 06 2022, Taylor Blau wrote:

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

I think it's OK if a *pending* patch is 10k lines or whatever, as long
as the result of applying it compiles. I.e. if it doesn't compile the
semantic patch is broken, and doesn't really help to clarify what the
proposed change is.

Whether the change itself is too large to be worth it or whatever is
another matter, but we can always consider it advisory.

E.g. consider a change to remove braces from if/else per our coding
style, we don't have such a *.cocci now, but if we did it would be a
very large patch. It might still be useful in *.pending.cocci, e.g. to
run new code through it.

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

Yes. A sweeping change like this should definitely start there (although
see below).

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

What I'm demonstrating with that commit is that you can make a
non-pending patch by blacklisting the functions, filenames etc. that are
known to be outstanding.

The advantage of doing that being that you'd apply it for any new code
outside those scopes.

So re the "see above" it's an explicit end-run around the "do we want to
apply this everywhere?".

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

I understand that's the goal, I just don't get how it's intuitive.

First we had REALLOC_ARRAY(), which is the same as a reallocarray(3)
where the "size" is computed for you based on the "ptr", which we know
to be a variable name in our case. Later we had ALLOC_ARRAY() and
CALLOC_ARRAY(), which aim to provide the same sort of interface for
malloc() and calloc().

The reallocarray() function comes from OpenBSD, where it's always
supported nmemb = 1:
https://man.openbsd.org/OpenBSD-5.9/malloc.3#reallocarray~2

It has always been redundant to use it for nmemb=1, but as arrays in C
don't have containers (unlike some higher-level lanugages) it would be
odd to disallow it, and force the user to do:

	mem = n == 1 ? realloc(mem, sz) : reallocarray(mem, n, sz);

Instead of:

	mem = reallocarray(mem, n, sz);

I.e. you don't need the overlfow check for n * sz if n == 1, but
treating it as a special-case is a hassle.