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

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

On Mon, Jul 11 2022, Jeff King wrote:

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

Thanks!

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

That's fair, and I suppose we could add some really complex tests that
e.g. include the original headers.

But right now these just work on stand-alone *.c files with no includes,
and the runtime is thus trivial. So I thought it was OK to run it along
with the main job.

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

Yeah, we definitely need to be more careful with "ongoing" rules like
these, but hopefully between the tests & lack of false positives this
one looks sane...

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