Re: [PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Peter Maydell]

On Wed, 1 Apr 2020 at 15:44, Markus Armbruster <address@hidden> wrote:
> Peter Maydell <address@hidden> writes:
> > On Wed, 1 Apr 2020 at 06:07, Markus Armbruster <address@hidden> wrote:
> > But then as a coccinelle script author I need to know which of
> > the options I needed are standard, which are for-this-script-only,
> > and which are just 'workflow'.
>
> If you're capable of writing a Coccinelle script that actually does what
> you want, I bet you dollars to donuts that you can decide which options
> actually affect the patch in comparably no time whatsoever ;)

I use this thing maybe once a month at most, more likely once
every three months, and the documentation is notoriously
impenetrable. I really really don't want to have to start looking in it
and guessing about how the original author ran the script, when
they could have just told me.

> If you prefer to bother your reader with your personal choices, that's
> between you and your reviewers.  Myself, I prefer less noise around the
> signal.

> If you got Coccinelle installed and know the very basics, then the
> incantation in the script should suffice to use the script, and the
> incantation in the commit message should suffice to reproduce the patch.

So I need to now look in the git log for the script to find the commit
message? Why not just put the command in the file and save steps?

> Example:
>
>     commit 4e20c1becba3fd2e8e71a2663cefb9627fd2a6e0
>     Author: Markus Armbruster <address@hidden>
>     Date:   Thu Dec 13 18:51:54 2018 +0100
>
>         block: Replace qdict_put() by qdict_put_obj() where appropriate
>
>         Patch created mechanically by rerunning:
>
>           $  spatch --sp-file scripts/coccinelle/qobject.cocci \
>                     --macro-file scripts/cocci-macro-file.h \
>                     --dir block --in-place

Yep, that command line would be great to see in the script file.

> scripts/coccinelle/qobject.cocci has no usage comment.  I doubt it needs
> one, but I'd certainly tolerate something like

    // Usage:
    // spatch --sp-file scripts/coccinelle/qobject.cocci \
    //        --macro-file scripts/cocci-macro-file.h \
    //        FILES ...

I think that should be about the minimum. I think every
.cocci file should say how it was used or is supposed to be used.
The least-effort way for the author of the script to do that is to
simply give the command line they used to run it.

> >       That's more work for the author *and* more work for the
> > reader than just "put the command line you used into the script
> > as a comment" -- so who's it benefiting?
>
> Anyone with basic Coccinelle proficiency benefits slightly from the
> reduction of noise.

How 'basic' is basic? I think that being specific is useful for
anybody who's at my level or lower (ie, can write a script, doesn't
do so often enough to be able to write a script or run spatch
without looking at documentation and cribbing from other scripts
as examples). How many people do we have at a higher level
than that for whom this is noise? 2? 3? And people who do
know Coccinelle well should have no difficulty in quickly
looking at a command line and mentally filtering out the options
that they don't feel they need.

thanks
-- PMM