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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 5, 2022, 23:12 UTC
Message-ID: <221206.868rjle7za.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y45yaYV3xFB/xR2G@nand.local>

On Mon, Dec 05 2022, Taylor Blau wrote:

Show 21 quoted lines

> On Mon, Dec 05, 2022 at 10:01:11PM +0100, René Scharfe wrote:
>> This rule would turn this code:
>>
>> 	struct foo *bar = xcalloc(1, sizeof(*bar));
>> 	int i;
>>
>> ... into:
>>
>> 	struct foo *bar;
>> 	CALLOC(bar);
>> 	int i;
>>
>> ... which violates the coding guideline to not mix declarations and
>> statements (-Wdeclaration-after-statement).
>
> Yeah, I was wondering about this myself when I wrote this part of the
> Coccinelle patch.
>
> Is there an intelligent way to tell it to put the first statement after
> all declarations? I couldn't find anything after a quick scan of the
> documentation nor our own patches.

We can get that working, but I think it's good to establish some ground rules first:

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.

In this case if I apply the proposed patch and do:

	make contrib/coccinelle/xcalloc.pending.cocci.patch &&
	patch -p1 <contrib/coccinelle/xcalloc.pending.cocci.patch &&
	make

We have a deluge of syntax errors, not just the warnings René notes. This needs to be fixed, the *.pending.cocci must actually work on our code.

(We should probably modify the "static-analysis" to actually apply the result of these *.pending patches and compile with those, but until then we can manually test them).

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.

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

I think that's preferable to having some *.pending.cocci we may or may not get to. I think you can also probably write a working rule for this to incrementally target certain subsets of these, and we could just mass-convert some stuff already (if it doesn't conflict with in-flight etc.).

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.e. I think the point of the macro should be to provide the appropiate sizeof() for you, not to be opinionated that nmemb=1 should be special-cased.I think it's probably not worth the churn to make that transformation.

But if we *are* doing that then surely we should provide the full set of functions. I.e. ALLOC() and ALLOC_ARRAY(), CALLOC() and CALLOC_ARRAY(), and REALLOC() and REALLOC_ARRAY()?