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

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 1 Apr 2020 at 06:07, Markus Armbruster <armbru@redhat.com> wrote:
>
>> Generic instructions for using .cocci scripts should go into README.
>> Enough to get you started if you know nothing about Coccinelle.
>>
>> Options that should always be used with a certain script should be
>> documented in that script.
>>
>> Options that only affect work-flow, not the patch, I'd rather keep out
>> of the script.  If there are any we feel we should mention, do that in
>> README.  Example: --no-show-diff.
>
> 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 ;)

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.

>                                And as a reader I *still* need to
> go and look through the README and look at this script and
> then try to reconstitute what command line might have been
> used.

You don't.  The "just work-flow" options by definition do not affect the
patch.

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.

If you know nothing about Coccinelle, the README should get you started.

Example:

    commit 4e20c1becba3fd2e8e71a2663cefb9627fd2a6e0
    Author: Markus Armbruster <armbru@redhat.com>
    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

        Signed-off-by: Markus Armbruster <armbru@redhat.com>
        Reviewed-by: Alberto Garcia <berto@igalia.com>
        Reviewed-by: Eric Blake <eblake@redhat.com>
        Signed-off-by: Kevin Wolf <kwolf@redhat.com>

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'd even tolerate throwing in --in-place.  But --use-gitgrep is too much
for my taste.

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

Coccinelle noobs benefit from the more verbose instructions in the
README.  Duplicating those in every script is not maintainable.

Maintainers benefit slightly from less redundancy.

>> Brief instructions for how a patch was created should be included in the
>> commit message.  They should suffice for readers familiar with
>> Coccinelle.  Yes, there's a bit of redundancy with README and the
>> script.