Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 28, 2017, 21:39 UTC
Message-ID: <87injfkd0j.fsf@gmail.com>
In-Reply-To: <2122b01f-7627-cd1b-c7df-751c076f90ff@web.de>

On Sun, Jun 25 2017, René Scharfe jotted:

Show 57 quoted lines

> Am 16.06.2017 um 21:43 schrieb Junio C Hamano:
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> A follow-up to the existing "type" rule added in an earlier
>>> change. This catches some occurrences that are missed by the previous
>>> rule.
>>>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> ---
>>
>> Hmph, I wonder if the "type" thing is really needed.  Over there,
>> "ptr" is an expression and we can find "free(ptr); ptr = NULL" with
>> the rule in this patch already, no?
>
> Indeed.  How about this on top of master?
>
> -- >8 --
> Subject: [PATCH] coccinelle: polish FREE_AND_NULL rules
>
> There are two rules for using FREE_AND_NULL in free.cocci, one for
> pointer types and one for expressions.  Both cause coccinelle to remove
> empty lines and even newline characters between replacements for some
> reason; consecutive "free(x);/x=NULL;" sequences end up as multiple
> FREE_AND_NULL calls on the same time.
>
> Remove the type rule, as the expression rule already covers it, and
> rearrange the lines of the latter to place the addition of FREE_AND_NULL
> between the removals, which causes coccinelle to leave surrounding
> whitespace untouched.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  contrib/coccinelle/free.cocci | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
> index f2d97e755b..4490069df9 100644
> --- a/contrib/coccinelle/free.cocci
> +++ b/contrib/coccinelle/free.cocci
> @@ -11,16 +11,8 @@ expression E;
>    free(E);
>
>  @@
> -type T;
> -T *ptr;
> -@@
> -- free(ptr);
> -- ptr = NULL;
> -+ FREE_AND_NULL(ptr);
> -
> -@@
>  expression E;
>  @@
>  - free(E);
> -- E = NULL;
>  + FREE_AND_NULL(E);
> +- E = NULL;

Late reply, sorry. What version of coccinelle are you running? I have 1.0.4 (from Debian) and can't get this to produce the same results as what I have.

On top of next, I did:

        Revert "*.[ch] refactoring: make use of the FREE_AND_NULL() macro"
        Revert "coccinelle: make use of the "expression" FREE_AND_NULL() rule"
        Revert "coccinelle: make use of the "type" FREE_AND_NULL() rule"

And then generated the patch as usual with `make coccicheck`, and applied it. This has your change.

I then re-applied the manual "*.[ch] refactoring" changes

This results in this diff with next:

    $ git diff --stat origin/next.. -- '*.[ch]'
     builtin/am.c             |  3 ++-
     builtin/clean.c          |  6 ++++--
     builtin/config.c         |  6 ++++--
     builtin/index-pack.c     |  6 ++++--
     builtin/pack-objects.c   | 12 ++++++++----
     builtin/unpack-objects.c |  3 ++-
     fast-import.c            |  6 ++++--
     http-push.c              | 24 ++++++++++++++++--------
     http.c                   | 15 ++++++++++-----
     imap-send.c              |  3 ++-
     ref-filter.c             |  3 ++-
     refs/files-backend.c     |  3 ++-
     remote-testsvn.c         |  3 ++-
     sequencer.c              |  3 ++-
     sha1-array.c             |  3 ++-
     sha1_file.c              |  3 ++-
     transport-helper.c       | 27 ++++++++++++++++++---------
     transport.c              |  3 ++-
     tree-diff.c              |  6 ++++--
     tree.c                   |  3 ++-
     20 files changed, 94 insertions(+), 47 deletions(-)

These are all cases where we now miss things that should use FREE_AND_NULL(), e.g.:

    diff --git a/builtin/am.c b/builtin/am.c
    index c973bd96dc..2f89338ed7 100644
    --- a/builtin/am.c
    +++ b/builtin/am.c
    @@ -484,7 +484,8 @@ static int run_applypatch_msg_hook(struct am_state *state)
            ret = run_hook_le(NULL, "applypatch-msg", am_path(state, "final-commit"), NULL);

            if (!ret) {
    -               FREE_AND_NULL(state->msg);
    +               free(state->msg);
    +               state->msg = NULL;
                    if (read_commit_msg(state) < 0)
                            die(_("'%s' was deleted by the applypatch-msg hook"),
                                    am_path(state, "final-commit"));

So it looks to me like removing the "type T" rule breaks a lot of things, but that the change you made to the expression rule is good, and we should do that for the "type" rule as well. Your commit says the "expression rule already covers it", but this doesn't seem to be the case at all.

As an aside: Junio, did you mean to apply f8bb4631fb to next this way? Looks like a mis-applied scissor commit.