From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mon, 05 Dec 2022 23:12:32 GMT
Subject: Re: [PATCH] git-compat-util.h: introduce CALLOC(x)
Message-ID: <221206.868rjle7za.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y45yaYV3xFB/xR2G@nand.local>


On Mon, Dec 05 2022, Taylor Blau wrote:

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