From: René Scharfe Date: Wed, 28 Jun 2017 22:30:12 GMT Subject: Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL() Message-ID: In-Reply-To: <433f9fb7-a202-1091-46de-04535b2a49e1@web.de> Am 29.06.2017 um 00:21 schrieb René Scharfe: > Am 28.06.2017 um 23:39 schrieb Ævar Arnfjörð Bjarmason: >> >> On Sun, Jun 25 2017, René Scharfe jotted: >> >>> Am 16.06.2017 um 21:43 schrieb Junio C Hamano: >>>> Ævar Arnfjörð Bjarmason 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 >>>>> --- >>>> >>>> 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 >>> --- >>> 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. > > I can't reproduce that strange result with master, but get a scissors > line into the commit message as well. The diff at the end looks > reasonable (contains just the manual stuff). Here's what I did: On top of next it looks basically the same for me, only difference being several new converted cases in repository.c. Did you commit before diff? René