Re: How do we review changes made with coccinelle?

[Taylor Blau <me@ttaylorr.com>]

On Thu, Mar 30, 2023 at 12:13:07PM -0700, Junio C Hamano wrote:
> Glen Choo <chooglen@google.com> writes:
>
> > - Is it okay to give Reviewed-By on the basis of _just_ the in-tree
> >   changes and ignore the .cocci patch?
>
> If they were made in separate steps, sure.  If not, not really.  But
> we can still say "I've checked the changes the author made to the
> code and they looked good."  But we haven't reviewed the patch in
> its entirety in such a case to give a Reviewed-by, I would thihk.

I think that while none of us would probably call ourselves "Coccinelle
experts", we are probably reasonably capable of reviewing *.cocci files
and their impact on the tree.

What I meant when we were talking about this off-list was that I don't
consider myself an expert at writing idiomatic Coccinelle rules. But I
feel competent enough that I could review Ævar's patches by roughly
grokking the *.cocci changes, and then checking that the resulting tree
state matched my understanding of those changes.

> > - Do we care about new patches slowing down coccicheck?

I was the one who asked this question off-list, and I did so in a
leading way that implied that the answer was "no".

> Surely.

But I agree with Junio that we *do* care about slowing down the
performance of 'make coccicheck'. When I originally asked, I was under
the (false) impression that we didn't run 'make coccicheck' in CI. But
we do (see ci/run-static-analysis.sh), so we do care about the
performance there since we don't want to unnecessarily slow down CI.

Thanks,
Taylor