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

From: Jeff King <peff@peff.net>
Date: Jul 11, 2022, 09:41 UTC
Message-ID: <YsvwLIfjZffUWOP/@coredump.intra.peff.net>
In-Reply-To: <cover-v4-0.6-00000000000-20220705T134033Z-avarab@gmail.com>

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

Show 5 quoted lines

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

Show 6 quoted lines

> 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