Re: [PATCH 2/2] cocci: codify authoring and reviewing practices

From: Glen Choo <chooglen@google.com>
Date: Apr 19, 2023, 19:29 UTC
Message-ID: <kl6lzg731xib.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230416074212.GB3271@szeder.dev>

SZEDER Gábor <szeder.dev@gmail.com> writes:

Show 10 quoted lines

>> +* .cocci rules should target only the problem it is trying to solve; "collateral
>> +  damage" is not allowed.
>> +
>> +* .cocci files used for refactoring should be temporarily kept in-tree to aid
>
> How should such semantic patches be kept in-tree?
> As .pending.cocci?  Then I think it would be better to point this out
> here.  Or as a "regular" semantic patch?  Then I'm not sure I agree
> with this recommendation, but perhaps a commit message explaining the
> reasoning behind this would help me make up my mind :)

I don't feel strongly about this, but I was envisioning keeping them as a "regular" patch, e.g. what Ævar proposed in:

  https://lore.kernel.org/git/230326.86ileow1fu.gmgdl@evledraar.gmail.com/

In theory, this means that a long running fork (that didn't get updated during the initial refactor) can run coccicheck, notice the failure, and then automatically fix themselves with the included semantic patch. In practice, I don't know how many forks run coccicheck, or whether these refactors are just easy enough to do by hand.

For refactors, I suspect that the impact on the 'make coccicheck' runtime will be low, since we're typically targeting just a few tokens and cocci can skip whatever files don't have those tokens, so keeping it as a "regular" patch might be okay.

> It might also be worth mentioning that before submitting a new
> semantic patch developers should consider its cost-benefit ratio, in
> particular its effect on the runtime of 'make coccicheck',

Makes sense, though I'm not sure what practical advice to give in order to evaluate the impact on runtime (besides just running it themselves).

> in the hope
> that we can avoid another 'unused.cocci' fiasco.

Maybe this is a good starting point for discussing cost-benefit analysis. I'm not familiar with this fiasco, though. Was an early version of 'unused.cocci' too broad, resulting in a massive hit to runtime?