Re: [PATCH v4 0/6] add and apply a rule to find "unused" init+free

[Jeff King <peff@peff.net>]

On Tue, Jul 05, 2022 at 03:46:54PM +0200, Ævar Arnfjörð Bjarmason wrote:

> This series adds a coccinelle rule to find and remove code where the
> only reference to a variable in a given function is to malloc() &
> free() it, where "malloc" and "free" also match
> "strbuf_init/strbuf_release", and then later in the series anything
> that looks like a init/free pattern.

As before, I'm perfectly happy with the actual transformations here.

> Changes since v3[1]
> 
>  * Add a "coccicheck-test" target in an early and new patch, the
>    structure mirrors that of coccinelle.git's own tests. As the
>    diffstat shows we have a *.c and *.res file which is C code
>    before/after a *.cocci rule is applied.

I have mixed feelings on this. I'm not opposed per se, and I even like
the fact that it provides examples of what we expect a rule to do. But I
worry about a world where the cost of using coccinelle goes up because
now transformations need to have tests along with them. The idea of
cocci is to make your life a little simpler and more efficient when
fixing up the code base (as opposed to perl, etc). But if it comes with
a bunch of boilerplate tasks, that might no longer be the case. I'm
already often on the fence about using cocci just because of the hours
I've sunk into debugging and puzzling things out.

I dunno. Maybe the bar is higher for stuff like this that we expect to
continue to find problems as time goes on (because it is not "here is a
transition", but "this is a common and easy mistake to make").

So again, I'm not really opposed to this patch in particular, but I want
to express caution at the direction we might be going, and at applying
new rules over-zealously.

>  * We now catch init/reset patterns as well as init/release fully
>    (i.e. for the "struct strbuf" early on)

Yeah, that makes sense.

-Peff