* [PATCH v3 1/4] cocci: add and apply a rule to find "unused" strbufs
2022-07-01 10:30 ` [PATCH v3 0/4] add and apply a rule to find "unused" init+free Ævar Arnfjörð Bjarmason
@ 2022-07-01 10:30 ` Ævar Arnfjörð Bjarmason
2022-07-01 18:04 ` Jeff King
2022-07-01 19:55 ` Eric Sunshine
2022-07-01 10:30 ` [PATCH v3 2/4] cocci: catch unused "strbuf" using an xmalloc() pattern Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
5 siblings, 2 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-01 10:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren,
Ævar Arnfjörð Bjarmason
Add a coccinelle rule to remove "struct strbuf" initialization
followed by calling "strbuf_release()" function.
See extensive commentary in the new "unused.cocci" for how it works,
and what it's intended to find and replace.
The inclusion of "contrib/scalar/scalar.c" is because "spatch" was
manually run on it (we don't usually run spatch on contrib).
The use of "with strict" here will be explained and amended in the
following commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/fetch.c | 3 +-
builtin/merge.c | 2 --
contrib/coccinelle/unused.cocci | 61 +++++++++++++++++++++++++++++++++
contrib/scalar/scalar.c | 3 +-
diff.c | 2 --
5 files changed, 63 insertions(+), 8 deletions(-)
create mode 100644 contrib/coccinelle/unused.cocci
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ac29c2b1ae3..8a3ae71fed0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1113,7 +1113,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
struct fetch_head *fetch_head, struct worktree **worktrees)
{
int url_len, i, rc = 0;
- struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
+ struct strbuf note = STRBUF_INIT;
const char *what, *kind;
struct ref *rm;
char *url;
@@ -1281,7 +1281,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
abort:
strbuf_release(¬e);
- strbuf_release(&err);
free(url);
return rc;
}
diff --git a/builtin/merge.c b/builtin/merge.c
index d9784d4891c..bbd70b17bc6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -502,7 +502,6 @@ static void merge_name(const char *remote, struct strbuf *msg)
{
struct commit *remote_head;
struct object_id branch_head;
- struct strbuf buf = STRBUF_INIT;
struct strbuf bname = STRBUF_INIT;
struct merge_remote_desc *desc;
const char *ptr;
@@ -590,7 +589,6 @@ static void merge_name(const char *remote, struct strbuf *msg)
oid_to_hex(&remote_head->object.oid), remote);
cleanup:
free(found_ref);
- strbuf_release(&buf);
strbuf_release(&bname);
}
diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
new file mode 100644
index 00000000000..bc26d39b313
--- /dev/null
+++ b/contrib/coccinelle/unused.cocci
@@ -0,0 +1,61 @@
+// This rule finds sequences of "unused" declerations and uses of
+// "struct strbuf".
+//
+// I.e. this finds cases where we only declare the variable, and then
+// release it, e.g.:
+//
+// struct strbuf buf = STRBUF_INIT;
+// [.. no other use of "buf" in the function ..]
+// strbuf_release(&buf)
+//
+// Or:
+//
+// struct strbuf buf;
+// [.. no other use of "buf" in the function ..]
+// strbuf_init(&buf, 0);
+// [.. no other use of "buf" in the function ..]
+// strbuf_release(&buf)
+//
+// To do do this we find (continued below)...
+@@
+type T;
+identifier I;
+// STRBUF_INIT
+constant INIT_MACRO =~ "^STRBUF_INIT$";
+// strbuf_init(&I, ...) etc.
+identifier INIT_CALL1 =~ "^strbuf_init$";
+// strbuf_release()
+identifier REL1 =~ "^strbuf_release$";
+@@
+
+// .. A declaration like "struct strbuf buf;"...
+(
+- T I;
+// ... or "struct strbuf buf = { 0 };" ...
+|
+- T I = { 0 };
+// ... or "struct STRBUF buf = STRBUF_INIT;" ...
+|
+- T I = INIT_MACRO;
+)
+
+// ... Optionally followed by lines that make no use of "buf", "&buf"
+// etc., but which ...
+<... when != \( I \| &I \)
+ when strict
+// .. (only) make use of "buf" or "&buf" to call something like
+// "strbuf_init(&buf, ...)" ...
+- \( INIT_CALL1 \)( \( I \| &I \), ...);
+...>
+
+// ... and then no mention of "buf" or "&buf" until we get to a
+// strbuf_release(&buf) at the end ...
+- \( REL1 \)( \( &I \| I \) );
+// ... and no use *after* either, e.g. we don't want to delete
+// init/strbuf_release() patterns, where "&buf" could be used
+// afterwards.
+ ... when != \( I \| &I \)
+ when strict
+// This rule also isn't capable of finding cases where &buf is used,
+// but only to e.g. pass that variable to a static function which
+// doesn't use it. The analysis is only function-local.
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 28176914e57..97e71fe19cd 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -687,7 +687,7 @@ static int cmd_diagnose(int argc, const char **argv)
int stdout_fd = -1, archiver_fd = -1;
time_t now = time(NULL);
struct tm tm;
- struct strbuf path = STRBUF_INIT, buf = STRBUF_INIT;
+ struct strbuf buf = STRBUF_INIT;
int res = 0;
argc = parse_options(argc, argv, NULL, options,
@@ -779,7 +779,6 @@ static int cmd_diagnose(int argc, const char **argv)
free(argv_copy);
strvec_clear(&archiver_args);
strbuf_release(&zip_path);
- strbuf_release(&path);
strbuf_release(&buf);
return res;
diff --git a/diff.c b/diff.c
index e71cf758861..d4290615aaa 100644
--- a/diff.c
+++ b/diff.c
@@ -1289,7 +1289,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
{
static const char *nneof = " No newline at end of file\n";
const char *context, *reset, *set, *set_sign, *meta, *fraginfo;
- struct strbuf sb = STRBUF_INIT;
enum diff_symbol s = eds->s;
const char *line = eds->line;
@@ -1521,7 +1520,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
default:
BUG("unknown diff symbol");
}
- strbuf_release(&sb);
}
static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,
--
2.37.0.900.g4d0de1cceb2
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 1/4] cocci: add and apply a rule to find "unused" strbufs
2022-07-01 10:30 ` [PATCH v3 1/4] cocci: add and apply a rule to find "unused" strbufs Ævar Arnfjörð Bjarmason
@ 2022-07-01 18:04 ` Jeff King
2022-07-01 19:55 ` Eric Sunshine
1 sibling, 0 replies; 24+ messages in thread
From: Jeff King @ 2022-07-01 18:04 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Elijah Newren
On Fri, Jul 01, 2022 at 12:30:56PM +0200, Ævar Arnfjörð Bjarmason wrote:
> The use of "with strict" here will be explained and amended in the
> following commit.
Really s/the following/a following/ now that there's another commit in
between. :)
I didn't find the comment there very enlightening, though. It says we're
dropping "with strict" to catch some new cases around early returns. But
it's not clear to me what "strict" does in the first place, why we'd
want it, or what we might be losing by dropping it.
I know, I can probably go read the coccinelle docs to figure it out, but
it might be worth saying why it's worth including in the first place
somewhere (either here or in the follow-up commit).
-Peff
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/4] cocci: add and apply a rule to find "unused" strbufs
2022-07-01 10:30 ` [PATCH v3 1/4] cocci: add and apply a rule to find "unused" strbufs Ævar Arnfjörð Bjarmason
2022-07-01 18:04 ` Jeff King
@ 2022-07-01 19:55 ` Eric Sunshine
1 sibling, 0 replies; 24+ messages in thread
From: Eric Sunshine @ 2022-07-01 19:55 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Derrick Stolee, Jeff King,
Elijah Newren
On Fri, Jul 1, 2022 at 6:36 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Add a coccinelle rule to remove "struct strbuf" initialization
> followed by calling "strbuf_release()" function.
I'm probably being overly pedantic, but I tripped over this failing to
mention that the strbuf is not used between the initialization and the
release. That is, I had expected it to say:
Add a coccinelle rule to remove "struct strbuf" initialization
followed by calling "strbuf_release()" function without any uses
of the strbuf in between.
> See extensive commentary in the new "unused.cocci" for how it works,
> and what it's intended to find and replace.
>
> The inclusion of "contrib/scalar/scalar.c" is because "spatch" was
> manually run on it (we don't usually run spatch on contrib).
>
> The use of "with strict" here will be explained and amended in the
> following commit.
Did you mean "when strict"?
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
> @@ -0,0 +1,61 @@
> +// This rule finds sequences of "unused" declerations and uses of
s/declerations/declarations/
> +// To do do this we find (continued below)...
s/do do/do/
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 2/4] cocci: catch unused "strbuf" using an xmalloc() pattern
2022-07-01 10:30 ` [PATCH v3 0/4] add and apply a rule to find "unused" init+free Ævar Arnfjörð Bjarmason
2022-07-01 10:30 ` [PATCH v3 1/4] cocci: add and apply a rule to find "unused" strbufs Ævar Arnfjörð Bjarmason
@ 2022-07-01 10:30 ` Ævar Arnfjörð Bjarmason
2022-07-01 10:30 ` [PATCH v3 3/4] cocci: remove "when strict" from unused.cocci Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
5 siblings, 0 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-01 10:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren,
Ævar Arnfjörð Bjarmason
There's no current matches for this rule, but it will match both:
struct strbuf *buf = xmalloc(sizeof(struct strbuf));
strbuf_init(buf, 0);
strbuf_release(buf);
And:
struct strbuf *buf;
buf = xmalloc(sizeof(struct strbuf));
strbuf_init(buf, 0);
strbuf_release(buf);
Note that we'd also match a strbuf_init() before the xmalloc(), but
we're not seeking to be so strict as to make checks that the compiler
will catch for us redundant, and saying we'll match either "init" or
"xmalloc" lines makes the rule simpler.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
contrib/coccinelle/unused.cocci | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
index bc26d39b313..43942f3cd4f 100644
--- a/contrib/coccinelle/unused.cocci
+++ b/contrib/coccinelle/unused.cocci
@@ -22,6 +22,8 @@ type T;
identifier I;
// STRBUF_INIT
constant INIT_MACRO =~ "^STRBUF_INIT$";
+// x[mc]alloc() etc.
+identifier MALLOC1 =~ "^x?[mc]alloc$";
// strbuf_init(&I, ...) etc.
identifier INIT_CALL1 =~ "^strbuf_init$";
// strbuf_release()
@@ -37,15 +39,25 @@ identifier REL1 =~ "^strbuf_release$";
// ... or "struct STRBUF buf = STRBUF_INIT;" ...
|
- T I = INIT_MACRO;
+|
+// ... or "struct strbuf *buf = xmalloc(...)" etc. ...
+- T I = MALLOC1(...);
)
// ... Optionally followed by lines that make no use of "buf", "&buf"
// etc., but which ...
<... when != \( I \| &I \)
when strict
+(
// .. (only) make use of "buf" or "&buf" to call something like
// "strbuf_init(&buf, ...)" ...
- \( INIT_CALL1 \)( \( I \| &I \), ...);
+|
+// .. or to follow-up a "struct strbuf *buf" with e.g. "buf =
+// xmalloc(...)" (which may in turn be followed-up by a
+// "strbuf_init()", which we'll match with INIT_CALL1) ...
+- I = MALLOC1(...);
+)
...>
// ... and then no mention of "buf" or "&buf" until we get to a
--
2.37.0.900.g4d0de1cceb2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 3/4] cocci: remove "when strict" from unused.cocci
2022-07-01 10:30 ` [PATCH v3 0/4] add and apply a rule to find "unused" init+free Ævar Arnfjörð Bjarmason
2022-07-01 10:30 ` [PATCH v3 1/4] cocci: add and apply a rule to find "unused" strbufs Ævar Arnfjörð Bjarmason
2022-07-01 10:30 ` [PATCH v3 2/4] cocci: catch unused "strbuf" using an xmalloc() pattern Ævar Arnfjörð Bjarmason
@ 2022-07-01 10:30 ` Ævar Arnfjörð Bjarmason
2022-07-01 21:33 ` Eric Sunshine
2022-07-01 10:30 ` [PATCH v3 4/4] cocci: generalize "unused" rule to cover more than "strbuf" Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-01 10:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren,
Ævar Arnfjörð Bjarmason
Remove the "when strict" constraint from the newly introduced rule to
find unused code. As seen in the change this will help us find cases
where a "return" was causing spatch in the middle of our match was
causing spatch to abort its analysis.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/merge.c | 2 --
contrib/coccinelle/unused.cocci | 2 --
2 files changed, 4 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index bbd70b17bc6..23170f2d2a6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -375,7 +375,6 @@ static void reset_hard(const struct object_id *oid, int verbose)
static void restore_state(const struct object_id *head,
const struct object_id *stash)
{
- struct strbuf sb = STRBUF_INIT;
const char *args[] = { "stash", "apply", NULL, NULL };
if (is_null_oid(stash))
@@ -391,7 +390,6 @@ static void restore_state(const struct object_id *head,
*/
run_command_v_opt(args, RUN_GIT_CMD);
- strbuf_release(&sb);
refresh_cache(REFRESH_QUIET);
}
diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
index 43942f3cd4f..9f0101c1350 100644
--- a/contrib/coccinelle/unused.cocci
+++ b/contrib/coccinelle/unused.cocci
@@ -47,7 +47,6 @@ identifier REL1 =~ "^strbuf_release$";
// ... Optionally followed by lines that make no use of "buf", "&buf"
// etc., but which ...
<... when != \( I \| &I \)
- when strict
(
// .. (only) make use of "buf" or "&buf" to call something like
// "strbuf_init(&buf, ...)" ...
@@ -67,7 +66,6 @@ identifier REL1 =~ "^strbuf_release$";
// init/strbuf_release() patterns, where "&buf" could be used
// afterwards.
... when != \( I \| &I \)
- when strict
// This rule also isn't capable of finding cases where &buf is used,
// but only to e.g. pass that variable to a static function which
// doesn't use it. The analysis is only function-local.
--
2.37.0.900.g4d0de1cceb2
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 3/4] cocci: remove "when strict" from unused.cocci
2022-07-01 10:30 ` [PATCH v3 3/4] cocci: remove "when strict" from unused.cocci Ævar Arnfjörð Bjarmason
@ 2022-07-01 21:33 ` Eric Sunshine
0 siblings, 0 replies; 24+ messages in thread
From: Eric Sunshine @ 2022-07-01 21:33 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Derrick Stolee, Jeff King,
Elijah Newren
On Fri, Jul 1, 2022 at 6:36 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Remove the "when strict" constraint from the newly introduced rule to
> find unused code. As seen in the change this will help us find cases
> where a "return" was causing spatch in the middle of our match was
> causing spatch to abort its analysis.
There seem to be some extraneous words in this paragraph. Did you mean:
Remove the "when strict" constraint from the newly introduced
rule to find unused code. As seen in the change this will help us
find cases where a "return" in the middle of our match was
causing spatch to abort its analysis.
?
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 4/4] cocci: generalize "unused" rule to cover more than "strbuf"
2022-07-01 10:30 ` [PATCH v3 0/4] add and apply a rule to find "unused" init+free Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-07-01 10:30 ` [PATCH v3 3/4] cocci: remove "when strict" from unused.cocci Ævar Arnfjörð Bjarmason
@ 2022-07-01 10:30 ` Ævar Arnfjörð Bjarmason
2022-07-01 18:09 ` [PATCH v3 0/4] add and apply a rule to find "unused" init+free Jeff King
2022-07-05 13:46 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
5 siblings, 0 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-01 10:30 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren,
Ævar Arnfjörð Bjarmason
Generalize the newly added "unused.cocci" rule to find more than just
"struct strbuf", let's have it find the same unused patterns for
"struct string_list", as well as other code that uses
similar-looking *_{release,clear,free}() and {release,clear,free}_*()
functions.
See [1] for example of code that would be covered by the
"get_worktrees()" part of this rule. We'd still need work that the
series is based on (we were passing "worktrees" to a function), but
could now do the change in [1] automatically.
1. https://lore.kernel.org/git/Yq6eJFUPPTv%2Fzc0o@coredump.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/repack.c | 2 --
contrib/coccinelle/unused.cocci | 31 ++++++++++++++++++++++++-------
2 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 4a7ae4cf489..482b66f57d6 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -727,7 +727,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
struct child_process cmd = CHILD_PROCESS_INIT;
struct string_list_item *item;
struct string_list names = STRING_LIST_INIT_DUP;
- struct string_list rollback = STRING_LIST_INIT_NODUP;
struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;
struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
struct pack_geometry *geometry = NULL;
@@ -1117,7 +1116,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
}
string_list_clear(&names, 0);
- string_list_clear(&rollback, 0);
string_list_clear(&existing_nonkept_packs, 0);
string_list_clear(&existing_kept_packs, 0);
clear_pack_geometry(geometry);
diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
index 9f0101c1350..a1b09d2d73d 100644
--- a/contrib/coccinelle/unused.cocci
+++ b/contrib/coccinelle/unused.cocci
@@ -1,5 +1,5 @@
// This rule finds sequences of "unused" declerations and uses of
-// "struct strbuf".
+// "struct strbuf" and other common types.
//
// I.e. this finds cases where we only declare the variable, and then
// release it, e.g.:
@@ -20,14 +20,18 @@
@@
type T;
identifier I;
-// STRBUF_INIT
-constant INIT_MACRO =~ "^STRBUF_INIT$";
+// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)
+constant INIT_MACRO =~ "_INIT";
// x[mc]alloc() etc.
identifier MALLOC1 =~ "^x?[mc]alloc$";
+// I = get_worktrees() etc.
+identifier INIT_ASSIGN1 =~ "^get_worktrees$";
// strbuf_init(&I, ...) etc.
-identifier INIT_CALL1 =~ "^strbuf_init$";
-// strbuf_release()
-identifier REL1 =~ "^strbuf_release$";
+identifier INIT_CALL1 =~ "^[a-z_]*_init$";
+// stbuf_release(), string_list_clear() etc.
+identifier REL1 =~ "^[a-z_]*_(release|clear|free)$";
+// release_patch(), clear_pathspec() etc.
+identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
@@
// .. A declaration like "struct strbuf buf;"...
@@ -52,6 +56,10 @@ identifier REL1 =~ "^strbuf_release$";
// "strbuf_init(&buf, ...)" ...
- \( INIT_CALL1 \)( \( I \| &I \), ...);
|
+// .. or e.g. "worktrees = get_worktrees();", i.e. a known "assignment
+// init" ...
+- I = \( INIT_ASSIGN1 \)(...);
+|
// .. or to follow-up a "struct strbuf *buf" with e.g. "buf =
// xmalloc(...)" (which may in turn be followed-up by a
// "strbuf_init()", which we'll match with INIT_CALL1) ...
@@ -61,11 +69,20 @@ identifier REL1 =~ "^strbuf_release$";
// ... and then no mention of "buf" or "&buf" until we get to a
// strbuf_release(&buf) at the end ...
-- \( REL1 \)( \( &I \| I \) );
+(
+- \( REL1 \| REL2 \)( \( I \| &I \), ...);
+|
+- \( REL1 \| REL2 \)( \( &I \| I \) );
+)
// ... and no use *after* either, e.g. we don't want to delete
// init/strbuf_release() patterns, where "&buf" could be used
// afterwards.
... when != \( I \| &I \)
+// Note that we're intentionally loose in accepting e.g. a
+// "strbuf_init(&buf)" followed by a "string_list_clear(&buf,
+// 0)". It's assumed that the compiler will catch any such invalid
+// code, i.e. that our constructors/destructors don't take a "void *".
+//
// This rule also isn't capable of finding cases where &buf is used,
// but only to e.g. pass that variable to a static function which
// doesn't use it. The analysis is only function-local.
--
2.37.0.900.g4d0de1cceb2
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 0/4] add and apply a rule to find "unused" init+free
2022-07-01 10:30 ` [PATCH v3 0/4] add and apply a rule to find "unused" init+free Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2022-07-01 10:30 ` [PATCH v3 4/4] cocci: generalize "unused" rule to cover more than "strbuf" Ævar Arnfjörð Bjarmason
@ 2022-07-01 18:09 ` Jeff King
2022-07-05 13:46 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
5 siblings, 0 replies; 24+ messages in thread
From: Jeff King @ 2022-07-01 18:09 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Elijah Newren
On Fri, Jul 01, 2022 at 12:30:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
> This series adds a coccinelle rule to find and remove code where the
> only reference to a variable in a given function is to malloc() &
> free() it, where "malloc" and "free" also match
> "strbuf_init/strbuf_release", and then later in the series anything
> that looks like a init/free pattern.
Overall, I hated this much less than I expected to. ;)
My main concern was that we'd have a jumble of init/release rules for
every data type, but the regex matching on identifier names lets us do
things broadly. And for data types that don't match our usual naming
conventions, that seems like a good opportunity to consider changing
their names.
I'm not too concerned about false positives here, since there are humans
in the loop. The resulting diffs are pretty obvious to review, and a
compiler would obviously catch any case where the variable really did
get used in between.
I do wish we could do it with the compiler and not coccinelle, just
because the latter has been such a source of frustration in terms of
performance issues and head-scratching "why didn't it catch this one"
cases. But this is IMHO much better than nothing, as shown by the
cleanups it already found.
-Peff
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v4 0/6] add and apply a rule to find "unused" init+free
2022-07-01 10:30 ` [PATCH v3 0/4] add and apply a rule to find "unused" init+free Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2022-07-01 18:09 ` [PATCH v3 0/4] add and apply a rule to find "unused" init+free Jeff King
@ 2022-07-05 13:46 ` Ævar Arnfjörð Bjarmason
2022-07-05 13:46 ` [PATCH v4 1/6] Makefile: remove mandatory "spatch" arguments from SPATCH_FLAGS Ævar Arnfjörð Bjarmason
` (7 more replies)
5 siblings, 8 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-05 13:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren,
Eric Sunshine, Ævar Arnfjörð Bjarmason
This series adds a coccinelle rule to find and remove code where the
only reference to a variable in a given function is to malloc() &
free() it, where "malloc" and "free" also match
"strbuf_init/strbuf_release", and then later in the series anything
that looks like a init/free pattern.
Changes since v3[1]
* Add a "coccicheck-test" target in an early and new patch, the
structure mirrors that of coccinelle.git's own tests. As the
diffstat shows we have a *.c and *.res file which is C code
before/after a *.cocci rule is applied.
* The extensive commentary in the proposed rule is gone in favor of
self-explanatory test cases
* We now catch init/reset patterns as well as init/release fully
(i.e. for the "struct strbuf" early on)
* Squashed the "when strict" change in, and there's now a test-case
for it.
* 1-2/6 are new minor cleanup patches for what follows.
1. https://lore.kernel.org/git/cover-v3-0.4-00000000000-20220701T102506Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (6):
Makefile: remove mandatory "spatch" arguments from SPATCH_FLAGS
Makefile & .gitignore: ignore & clean "git.res", not "*.res"
cocci: add a "coccicheck-test" target and test *.cocci rules
cocci: have "coccicheck{,-pending}" depend on "coccicheck-test"
cocci: add and apply a rule to find "unused" strbufs
cocci: generalize "unused" rule to cover more than "strbuf"
.gitignore | 2 +-
Makefile | 28 ++++++++--
builtin/fetch.c | 3 +-
builtin/merge.c | 4 --
builtin/repack.c | 2 -
contrib/coccinelle/tests/free.c | 11 ++++
contrib/coccinelle/tests/free.res | 9 ++++
contrib/coccinelle/tests/unused.c | 82 +++++++++++++++++++++++++++++
contrib/coccinelle/tests/unused.res | 45 ++++++++++++++++
contrib/coccinelle/unused.cocci | 43 +++++++++++++++
contrib/scalar/scalar.c | 3 +-
diff.c | 2 -
shared.mak | 1 +
13 files changed, 219 insertions(+), 16 deletions(-)
create mode 100644 contrib/coccinelle/tests/free.c
create mode 100644 contrib/coccinelle/tests/free.res
create mode 100644 contrib/coccinelle/tests/unused.c
create mode 100644 contrib/coccinelle/tests/unused.res
create mode 100644 contrib/coccinelle/unused.cocci
Range-diff against v3:
-: ----------- > 1: fbdc2c3d66b Makefile: remove mandatory "spatch" arguments from SPATCH_FLAGS
-: ----------- > 2: d7e85d4c4a6 Makefile & .gitignore: ignore & clean "git.res", not "*.res"
-: ----------- > 3: 540186e69dc cocci: add a "coccicheck-test" target and test *.cocci rules
-: ----------- > 4: 48810f7390c cocci: have "coccicheck{,-pending}" depend on "coccicheck-test"
1: 49e9ccb5819 ! 5: d1c6833c8d5 cocci: add and apply a rule to find "unused" strbufs
@@ Commit message
cocci: add and apply a rule to find "unused" strbufs
Add a coccinelle rule to remove "struct strbuf" initialization
- followed by calling "strbuf_release()" function.
+ followed by calling "strbuf_release()" function, without any uses of
+ the strbuf in the same function.
- See extensive commentary in the new "unused.cocci" for how it works,
- and what it's intended to find and replace.
+ See the tests in contrib/coccinelle/tests/unused.{c,res} for what it's
+ intended to find and replace.
The inclusion of "contrib/scalar/scalar.c" is because "spatch" was
manually run on it (we don't usually run spatch on contrib).
- The use of "with strict" here will be explained and amended in the
- following commit.
+ Per the "buggy code" comment we also match a strbuf_init() before the
+ xmalloc(), but we're not seeking to be so strict as to make checks
+ that the compiler will catch for us redundant. Saying we'll match
+ either "init" or "xmalloc" lines makes the rule simpler.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
@@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *
}
## builtin/merge.c ##
+@@ builtin/merge.c: static void reset_hard(const struct object_id *oid, int verbose)
+ static void restore_state(const struct object_id *head,
+ const struct object_id *stash)
+ {
+- struct strbuf sb = STRBUF_INIT;
+ const char *args[] = { "stash", "apply", NULL, NULL };
+
+ if (is_null_oid(stash))
+@@ builtin/merge.c: static void restore_state(const struct object_id *head,
+ */
+ run_command_v_opt(args, RUN_GIT_CMD);
+
+- strbuf_release(&sb);
+ refresh_cache(REFRESH_QUIET);
+ }
+
@@ builtin/merge.c: static void merge_name(const char *remote, struct strbuf *msg)
{
struct commit *remote_head;
@@ builtin/merge.c: static void merge_name(const char *remote, struct strbuf *msg)
}
+ ## contrib/coccinelle/tests/unused.c (new) ##
+@@
++void test_strbuf(void)
++{
++ struct strbuf sb1 = STRBUF_INIT;
++ struct strbuf sb2 = STRBUF_INIT;
++ struct strbuf sb3 = STRBUF_INIT;
++ struct strbuf sb4 = STRBUF_INIT;
++ struct strbuf sb5;
++ struct strbuf sb6 = { 0 };
++ struct strbuf sb7 = STRBUF_INIT;
++ struct strbuf sb8 = STRBUF_INIT;
++ struct strbuf *sp1;
++ struct strbuf *sp2;
++ struct strbuf *sp3;
++ struct strbuf *sp4 = xmalloc(sizeof(struct strbuf));
++ struct strbuf *sp5 = xmalloc(sizeof(struct strbuf));
++ struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
++ struct strbuf *sp7;
++
++ strbuf_init(&sb5, 0);
++ strbuf_init(sp1, 0);
++ strbuf_init(sp2, 0);
++ strbuf_init(sp3, 0);
++ strbuf_init(sp4, 0);
++ strbuf_init(sp5, 0);
++ strbuf_init(sp6, 0);
++ strbuf_init(sp7, 0);
++ sp7 = xmalloc(sizeof(struct strbuf));
++
++ use_before(&sb3);
++ use_as_str("%s", sb7.buf);
++ use_as_str("%s", sp1->buf);
++ use_as_str("%s", sp6->buf);
++ pass_pp(&sp3);
++
++ strbuf_release(&sb1);
++ strbuf_reset(&sb2);
++ strbuf_release(&sb3);
++ strbuf_release(&sb4);
++ strbuf_release(&sb5);
++ strbuf_release(&sb6);
++ strbuf_release(&sb7);
++ strbuf_release(sp1);
++ strbuf_release(sp2);
++ strbuf_release(sp3);
++ strbuf_release(sp4);
++ strbuf_release(sp5);
++ strbuf_release(sp6);
++ strbuf_release(sp7);
++
++ use_after(&sb4);
++
++ if (when_strict())
++ return;
++ strbuf_release(&sb8);
++}
+
+ ## contrib/coccinelle/tests/unused.res (new) ##
+@@
++void test_strbuf(void)
++{
++ struct strbuf sb3 = STRBUF_INIT;
++ struct strbuf sb4 = STRBUF_INIT;
++ struct strbuf sb7 = STRBUF_INIT;
++ struct strbuf *sp1;
++ struct strbuf *sp3;
++ struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
++ strbuf_init(sp1, 0);
++ strbuf_init(sp3, 0);
++ strbuf_init(sp6, 0);
++
++ use_before(&sb3);
++ use_as_str("%s", sb7.buf);
++ use_as_str("%s", sp1->buf);
++ use_as_str("%s", sp6->buf);
++ pass_pp(&sp3);
++
++ strbuf_release(&sb3);
++ strbuf_release(&sb4);
++ strbuf_release(&sb7);
++ strbuf_release(sp1);
++ strbuf_release(sp3);
++ strbuf_release(sp6);
++
++ use_after(&sb4);
++
++ if (when_strict())
++ return;
++}
+
## contrib/coccinelle/unused.cocci (new) ##
@@
-+// This rule finds sequences of "unused" declerations and uses of
-+// "struct strbuf".
-+//
-+// I.e. this finds cases where we only declare the variable, and then
-+// release it, e.g.:
-+//
-+// struct strbuf buf = STRBUF_INIT;
-+// [.. no other use of "buf" in the function ..]
-+// strbuf_release(&buf)
-+//
-+// Or:
-+//
-+// struct strbuf buf;
-+// [.. no other use of "buf" in the function ..]
-+// strbuf_init(&buf, 0);
-+// [.. no other use of "buf" in the function ..]
-+// strbuf_release(&buf)
-+//
-+// To do do this we find (continued below)...
++// This rule finds sequences of "unused" declerations and uses of a
++// variable, where "unused" is defined to include only calling the
++// equivalent of alloc, init & free functions on the variable.
+@@
+type T;
+identifier I;
-+// STRBUF_INIT
+constant INIT_MACRO =~ "^STRBUF_INIT$";
-+// strbuf_init(&I, ...) etc.
++identifier MALLOC1 =~ "^x?[mc]alloc$";
+identifier INIT_CALL1 =~ "^strbuf_init$";
-+// strbuf_release()
-+identifier REL1 =~ "^strbuf_release$";
++identifier REL1 =~ "^strbuf_(release|reset)$";
+@@
+
-+// .. A declaration like "struct strbuf buf;"...
+(
+- T I;
-+// ... or "struct strbuf buf = { 0 };" ...
+|
+- T I = { 0 };
-+// ... or "struct STRBUF buf = STRBUF_INIT;" ...
+|
+- T I = INIT_MACRO;
++|
++- T I = MALLOC1(...);
+)
+
-+// ... Optionally followed by lines that make no use of "buf", "&buf"
-+// etc., but which ...
+<... when != \( I \| &I \)
-+ when strict
-+// .. (only) make use of "buf" or "&buf" to call something like
-+// "strbuf_init(&buf, ...)" ...
++(
+- \( INIT_CALL1 \)( \( I \| &I \), ...);
++|
++- I = MALLOC1(...);
++)
+...>
+
-+// ... and then no mention of "buf" or "&buf" until we get to a
-+// strbuf_release(&buf) at the end ...
+- \( REL1 \)( \( &I \| I \) );
-+// ... and no use *after* either, e.g. we don't want to delete
-+// init/strbuf_release() patterns, where "&buf" could be used
-+// afterwards.
+ ... when != \( I \| &I \)
-+ when strict
-+// This rule also isn't capable of finding cases where &buf is used,
-+// but only to e.g. pass that variable to a static function which
-+// doesn't use it. The analysis is only function-local.
## contrib/scalar/scalar.c ##
@@ contrib/scalar/scalar.c: static int cmd_diagnose(int argc, const char **argv)
2: 6324d3956ed < -: ----------- cocci: catch unused "strbuf" using an xmalloc() pattern
3: 9a5e7208dec < -: ----------- cocci: remove "when strict" from unused.cocci
4: 45a429b9cc9 ! 6: dafd59d5ded cocci: generalize "unused" rule to cover more than "strbuf"
@@ Commit message
similar-looking *_{release,clear,free}() and {release,clear,free}_*()
functions.
+ We're intentionally loose in accepting e.g. a "strbuf_init(&sb)"
+ followed by a "string_list_clear(&sb, 0)". It's assumed that the
+ compiler will catch any such invalid code, i.e. that our
+ constructors/destructors don't take a "void *".
+
See [1] for example of code that would be covered by the
"get_worktrees()" part of this rule. We'd still need work that the
series is based on (we were passing "worktrees" to a function), but
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
string_list_clear(&existing_kept_packs, 0);
clear_pack_geometry(geometry);
+ ## contrib/coccinelle/tests/unused.c ##
+@@ contrib/coccinelle/tests/unused.c: void test_strbuf(void)
+ return;
+ strbuf_release(&sb8);
+ }
++
++void test_other(void)
++{
++ struct string_list l = STRING_LIST_INIT_DUP;
++ struct strbuf sb = STRBUF_INIT;
++
++ string_list_clear(&l, 0);
++ string_list_clear(&sb, 0);
++}
++
++void test_worktrees(void)
++{
++ struct worktree **w1 = get_worktrees();
++ struct worktree **w2 = get_worktrees();
++ struct worktree **w3;
++ struct worktree **w4;
++
++ w3 = get_worktrees();
++ w4 = get_worktrees();
++
++ use_it(w4);
++
++ free_worktrees(w1);
++ free_worktrees(w2);
++ free_worktrees(w3);
++ free_worktrees(w4);
++}
+
+ ## contrib/coccinelle/tests/unused.res ##
+@@ contrib/coccinelle/tests/unused.res: void test_strbuf(void)
+ if (when_strict())
+ return;
+ }
++
++void test_other(void)
++{
++}
++
++void test_worktrees(void)
++{
++ struct worktree **w4;
++
++ w4 = get_worktrees();
++
++ use_it(w4);
++
++ free_worktrees(w4);
++}
+
## contrib/coccinelle/unused.cocci ##
-@@
- // This rule finds sequences of "unused" declerations and uses of
--// "struct strbuf".
-+// "struct strbuf" and other common types.
- //
- // I.e. this finds cases where we only declare the variable, and then
- // release it, e.g.:
@@
@@
type T;
identifier I;
--// STRBUF_INIT
-constant INIT_MACRO =~ "^STRBUF_INIT$";
+// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)
+constant INIT_MACRO =~ "_INIT";
- // x[mc]alloc() etc.
identifier MALLOC1 =~ "^x?[mc]alloc$";
-+// I = get_worktrees() etc.
-+identifier INIT_ASSIGN1 =~ "^get_worktrees$";
- // strbuf_init(&I, ...) etc.
-identifier INIT_CALL1 =~ "^strbuf_init$";
--// strbuf_release()
--identifier REL1 =~ "^strbuf_release$";
+-identifier REL1 =~ "^strbuf_(release|reset)$";
++identifier INIT_ASSIGN1 =~ "^get_worktrees$";
+identifier INIT_CALL1 =~ "^[a-z_]*_init$";
-+// stbuf_release(), string_list_clear() etc.
-+identifier REL1 =~ "^[a-z_]*_(release|clear|free)$";
-+// release_patch(), clear_pathspec() etc.
++identifier REL1 =~ "^[a-z_]*_(release|reset|clear|free)$";
+identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
@@
- // .. A declaration like "struct strbuf buf;"...
-@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ "^strbuf_release$";
- // "strbuf_init(&buf, ...)" ...
+ (
+@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ "^strbuf_(release|reset)$";
+ - T I = INIT_MACRO;
+ |
+ - T I = MALLOC1(...);
++|
++- T I = INIT_ASSIGN1(...);
+ )
+
+ <... when != \( I \| &I \)
+ (
- \( INIT_CALL1 \)( \( I \| &I \), ...);
|
-+// .. or e.g. "worktrees = get_worktrees();", i.e. a known "assignment
-+// init" ...
+- I = \( INIT_ASSIGN1 \)(...);
+|
- // .. or to follow-up a "struct strbuf *buf" with e.g. "buf =
- // xmalloc(...)" (which may in turn be followed-up by a
- // "strbuf_init()", which we'll match with INIT_CALL1) ...
-@@ contrib/coccinelle/unused.cocci: identifier REL1 =~ "^strbuf_release$";
+ - I = MALLOC1(...);
+ )
+ ...>
- // ... and then no mention of "buf" or "&buf" until we get to a
- // strbuf_release(&buf) at the end ...
-- \( REL1 \)( \( &I \| I \) );
+(
+- \( REL1 \| REL2 \)( \( I \| &I \), ...);
+|
+- \( REL1 \| REL2 \)( \( &I \| I \) );
+)
- // ... and no use *after* either, e.g. we don't want to delete
- // init/strbuf_release() patterns, where "&buf" could be used
- // afterwards.
... when != \( I \| &I \)
-+// Note that we're intentionally loose in accepting e.g. a
-+// "strbuf_init(&buf)" followed by a "string_list_clear(&buf,
-+// 0)". It's assumed that the compiler will catch any such invalid
-+// code, i.e. that our constructors/destructors don't take a "void *".
-+//
- // This rule also isn't capable of finding cases where &buf is used,
- // but only to e.g. pass that variable to a static function which
- // doesn't use it. The analysis is only function-local.
--
2.37.0.913.g50625c3f077
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v4 1/6] Makefile: remove mandatory "spatch" arguments from SPATCH_FLAGS
2022-07-05 13:46 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
@ 2022-07-05 13:46 ` Ævar Arnfjörð Bjarmason
2022-07-05 13:46 ` [PATCH v4 2/6] Makefile & .gitignore: ignore & clean "git.res", not "*.res" Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-05 13:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren,
Eric Sunshine, Ævar Arnfjörð Bjarmason
The "--patch ." part of SPATCH_FLAGS added in f57d11728d1 (coccinelle:
put sane filenames into output patches, 2018-07-23) should have been
added unconditionally to the "spatch" invocation instead, using it
isn't optional.
Let's also move the other mandatory flag to come after
$(SPATCH_FLAGS), to ensure that our "--sp-file" overrides any provided
in the environment, both --sp-file <arg> and --patch <arg> are
last-option-wins as far as spatch(1) option parsing is concerned.
The environment variable override was initially added in
a9a884aea57 (coccicheck: use --all-includes by default,
2016-09-30). In practice there's probably nobody that's using
SPATCH_FLAGS to try to intentionally break our invocations, but since
we're changing this let's make it clear what (if anything) we expect
to be overridden by user-supplied flags.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Makefile | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 04d0fd1fe60..c1d02f04499 100644
--- a/Makefile
+++ b/Makefile
@@ -1286,7 +1286,7 @@ SANITIZE_ADDRESS =
# For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
# usually result in less CPU usage at the cost of higher peak memory.
# Setting it to 0 will feed all files in a single spatch invocation.
-SPATCH_FLAGS = --all-includes --patch .
+SPATCH_FLAGS = --all-includes
SPATCH_BATCH_SIZE = 1
include config.mak.uname
@@ -3131,7 +3131,8 @@ check: $(GENERATED_H)
limit='-n $(SPATCH_BATCH_SIZE)'; \
fi; \
if ! echo $(COCCI_SOURCES) | xargs $$limit \
- $(SPATCH) --sp-file $< $(SPATCH_FLAGS) \
+ $(SPATCH) $(SPATCH_FLAGS) \
+ --sp-file $< --patch . \
>$@+ 2>$@.log; \
then \
cat $@.log; \
--
2.37.0.913.g50625c3f077
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 2/6] Makefile & .gitignore: ignore & clean "git.res", not "*.res"
2022-07-05 13:46 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
2022-07-05 13:46 ` [PATCH v4 1/6] Makefile: remove mandatory "spatch" arguments from SPATCH_FLAGS Ævar Arnfjörð Bjarmason
@ 2022-07-05 13:46 ` Ævar Arnfjörð Bjarmason
2022-07-05 13:46 ` [PATCH v4 3/6] cocci: add a "coccicheck-test" target and test *.cocci rules Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-05 13:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Adjust the overly broad .gitignore and "make clean" rule added in
ce39c2e04ce (Provide a Windows version resource for the git
executables., 2012-05-24).
For now this is merely a correctness fix, but needed because a
subsequent commit will want to check in *.res files elsewhere in the
tree, which we shouldn't have to "git add -f".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
.gitignore | 2 +-
Makefile | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/.gitignore b/.gitignore
index a4522157641..42fd7253b44 100644
--- a/.gitignore
+++ b/.gitignore
@@ -185,6 +185,7 @@
/git-worktree
/git-write-tree
/git-core-*/?*
+/git.res
/gitweb/GITWEB-BUILD-OPTIONS
/gitweb/gitweb.cgi
/gitweb/static/gitweb.js
@@ -225,7 +226,6 @@
*.hcc
*.obj
*.lib
-*.res
*.sln
*.sp
*.suo
diff --git a/Makefile b/Makefile
index c1d02f04499..1ccf13595de 100644
--- a/Makefile
+++ b/Makefile
@@ -3409,7 +3409,7 @@ cocciclean:
clean: profile-clean coverage-clean cocciclean
$(RM) -r .build
$(RM) po/git.pot po/git-core.pot
- $(RM) *.res
+ $(RM) git.res
$(RM) $(OBJECTS)
$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
--
2.37.0.913.g50625c3f077
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 3/6] cocci: add a "coccicheck-test" target and test *.cocci rules
2022-07-05 13:46 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
2022-07-05 13:46 ` [PATCH v4 1/6] Makefile: remove mandatory "spatch" arguments from SPATCH_FLAGS Ævar Arnfjörð Bjarmason
2022-07-05 13:46 ` [PATCH v4 2/6] Makefile & .gitignore: ignore & clean "git.res", not "*.res" Ævar Arnfjörð Bjarmason
@ 2022-07-05 13:46 ` Ævar Arnfjörð Bjarmason
2022-07-05 13:46 ` [PATCH v4 4/6] cocci: have "coccicheck{,-pending}" depend on "coccicheck-test" Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-05 13:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Add a "coccicheck-test" target to test our *.cocci rules, and as a
demonstration add tests for the rules added in 39ea59a2570 (remove
unnecessary NULL check before free(3), 2016-10-08) and
1b83d1251ed (coccinelle: add a rule to make "expression" code use
FREE_AND_NULL(), 2017-06-15).
I considered making use of the "spatch --test" option, and the choice
of a "tests" over a "t" directory is to make these tests compatible
with such a future change.
Unfortunately "spatch --test" doesn't return meaningful exit codes,
AFAICT you need to "grep" its output to see if the *.res is what you
expect. There's "--test-okfailed", but I didn't find a way to sensibly
integrate those (it relies on some in-between status files, but
doesn't help with the status codes).
Instead let's use a "--sp-file" pattern similar to the main
"coccicheck" rule, with the difference that we use and compare the
two *.res files with cmp(1).
The --very-quiet and --no-show-diff options ensure that we don't need
to pipe stdout and stderr somewhere. Unlike the "%.cocci.patch" rule
we're not using the diff.
The "cmp || git diff" is optimistically giving us better output on
failure, but even if we only have POSIX cmp and no system git
installed we'll still fail with the "cmp", just with an error message
that isn't as friendly. The "2>/dev/null" is in case we don't have a
"git" installed.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Makefile | 19 +++++++++++++++++++
contrib/coccinelle/tests/free.c | 11 +++++++++++
contrib/coccinelle/tests/free.res | 9 +++++++++
shared.mak | 1 +
4 files changed, 40 insertions(+)
create mode 100644 contrib/coccinelle/tests/free.c
create mode 100644 contrib/coccinelle/tests/free.res
diff --git a/Makefile b/Makefile
index 1ccf13595de..c4064f8394f 100644
--- a/Makefile
+++ b/Makefile
@@ -3123,6 +3123,8 @@ check: $(GENERATED_H)
exit 1; \
fi
+COCCI_TEST_RES = $(wildcard contrib/coccinelle/tests/*.res)
+
%.cocci.patch: %.cocci $(COCCI_SOURCES)
$(QUIET_SPATCH) \
if test $(SPATCH_BATCH_SIZE) = 0; then \
@@ -3143,6 +3145,22 @@ check: $(GENERATED_H)
then \
echo ' ' SPATCH result: $@; \
fi
+
+COCCI_TEST_RES_GEN = $(addprefix .build/,$(COCCI_TEST_RES))
+$(COCCI_TEST_RES_GEN): .build/%.res : %.c
+$(COCCI_TEST_RES_GEN): .build/%.res : %.res
+$(COCCI_TEST_RES_GEN): .build/contrib/coccinelle/tests/%.res : contrib/coccinelle/%.cocci
+ $(call mkdir_p_parent_template)
+ $(QUIET_SPATCH_T)$(SPATCH) $(SPATCH_FLAGS) \
+ --very-quiet --no-show-diff \
+ --sp-file $< -o $@ \
+ $(@:.build/%.res=%.c) && \
+ cmp $(@:.build/%=%) $@ || \
+ git -P diff --no-index $(@:.build/%=%) $@ 2>/dev/null; \
+
+.PHONY: coccicheck-test
+coccicheck-test: $(COCCI_TEST_RES_GEN)
+
coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))
# See contrib/coccinelle/README
@@ -3404,6 +3422,7 @@ profile-clean:
$(RM) $(addsuffix *.gcno,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))
cocciclean:
+ $(RM) -r .build/contrib/coccinelle
$(RM) contrib/coccinelle/*.cocci.patch*
clean: profile-clean coverage-clean cocciclean
diff --git a/contrib/coccinelle/tests/free.c b/contrib/coccinelle/tests/free.c
new file mode 100644
index 00000000000..96d4abc0c78
--- /dev/null
+++ b/contrib/coccinelle/tests/free.c
@@ -0,0 +1,11 @@
+int use_FREE_AND_NULL(int *v)
+{
+ free(*v);
+ *v = NULL;
+}
+
+int need_no_if(int *v)
+{
+ if (v)
+ free(v);
+}
diff --git a/contrib/coccinelle/tests/free.res b/contrib/coccinelle/tests/free.res
new file mode 100644
index 00000000000..f90fd9f48e3
--- /dev/null
+++ b/contrib/coccinelle/tests/free.res
@@ -0,0 +1,9 @@
+int use_FREE_AND_NULL(int *v)
+{
+ FREE_AND_NULL(*v);
+}
+
+int need_no_if(int *v)
+{
+ free(v);
+}
diff --git a/shared.mak b/shared.mak
index 4330192e9c3..33f43edbf9a 100644
--- a/shared.mak
+++ b/shared.mak
@@ -70,6 +70,7 @@ ifndef V
QUIET_HDR = @echo ' ' HDR $(<:hcc=h);
QUIET_RC = @echo ' ' RC $@;
QUIET_SPATCH = @echo ' ' SPATCH $<;
+ QUIET_SPATCH_T = @echo ' ' SPATCH TEST $(@:.build/%=%);
## Used in "Documentation/Makefile"
QUIET_ASCIIDOC = @echo ' ' ASCIIDOC $@;
--
2.37.0.913.g50625c3f077
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 4/6] cocci: have "coccicheck{,-pending}" depend on "coccicheck-test"
2022-07-05 13:46 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-07-05 13:46 ` [PATCH v4 3/6] cocci: add a "coccicheck-test" target and test *.cocci rules Ævar Arnfjörð Bjarmason
@ 2022-07-05 13:46 ` Ævar Arnfjörð Bjarmason
2022-07-05 13:46 ` [PATCH v4 5/6] cocci: add and apply a rule to find "unused" strbufs Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-05 13:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Have the newly introduced "coccicheck-test" target run implicitly when
"coccicheck" itself is run. As with e.g. the "check-chainlint"
target (see [1]) it makes sense to run this unconditionally before we
run other "spatch" rules as a basic sanity check. See
1. 803394459d4 (t/Makefile: add machinery to check correctness of
chainlint.sed, 2018-07-11)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Makefile b/Makefile
index c4064f8394f..669828a2a6b 100644
--- a/Makefile
+++ b/Makefile
@@ -3161,9 +3161,11 @@ $(COCCI_TEST_RES_GEN): .build/contrib/coccinelle/tests/%.res : contrib/coccinell
.PHONY: coccicheck-test
coccicheck-test: $(COCCI_TEST_RES_GEN)
+coccicheck: coccicheck-test
coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))
# See contrib/coccinelle/README
+coccicheck-pending: coccicheck-test
coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))
.PHONY: coccicheck coccicheck-pending
--
2.37.0.913.g50625c3f077
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 5/6] cocci: add and apply a rule to find "unused" strbufs
2022-07-05 13:46 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2022-07-05 13:46 ` [PATCH v4 4/6] cocci: have "coccicheck{,-pending}" depend on "coccicheck-test" Ævar Arnfjörð Bjarmason
@ 2022-07-05 13:46 ` Ævar Arnfjörð Bjarmason
2022-07-05 13:47 ` [PATCH v4 6/6] cocci: generalize "unused" rule to cover more than "strbuf" Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-05 13:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Add a coccinelle rule to remove "struct strbuf" initialization
followed by calling "strbuf_release()" function, without any uses of
the strbuf in the same function.
See the tests in contrib/coccinelle/tests/unused.{c,res} for what it's
intended to find and replace.
The inclusion of "contrib/scalar/scalar.c" is because "spatch" was
manually run on it (we don't usually run spatch on contrib).
Per the "buggy code" comment we also match a strbuf_init() before the
xmalloc(), but we're not seeking to be so strict as to make checks
that the compiler will catch for us redundant. Saying we'll match
either "init" or "xmalloc" lines makes the rule simpler.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/fetch.c | 3 +-
builtin/merge.c | 4 ---
contrib/coccinelle/tests/unused.c | 55 +++++++++++++++++++++++++++++
contrib/coccinelle/tests/unused.res | 30 ++++++++++++++++
contrib/coccinelle/unused.cocci | 32 +++++++++++++++++
contrib/scalar/scalar.c | 3 +-
diff.c | 2 --
7 files changed, 119 insertions(+), 10 deletions(-)
create mode 100644 contrib/coccinelle/tests/unused.c
create mode 100644 contrib/coccinelle/tests/unused.res
create mode 100644 contrib/coccinelle/unused.cocci
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ac29c2b1ae3..8a3ae71fed0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1113,7 +1113,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
struct fetch_head *fetch_head, struct worktree **worktrees)
{
int url_len, i, rc = 0;
- struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
+ struct strbuf note = STRBUF_INIT;
const char *what, *kind;
struct ref *rm;
char *url;
@@ -1281,7 +1281,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
abort:
strbuf_release(¬e);
- strbuf_release(&err);
free(url);
return rc;
}
diff --git a/builtin/merge.c b/builtin/merge.c
index d9784d4891c..23170f2d2a6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -375,7 +375,6 @@ static void reset_hard(const struct object_id *oid, int verbose)
static void restore_state(const struct object_id *head,
const struct object_id *stash)
{
- struct strbuf sb = STRBUF_INIT;
const char *args[] = { "stash", "apply", NULL, NULL };
if (is_null_oid(stash))
@@ -391,7 +390,6 @@ static void restore_state(const struct object_id *head,
*/
run_command_v_opt(args, RUN_GIT_CMD);
- strbuf_release(&sb);
refresh_cache(REFRESH_QUIET);
}
@@ -502,7 +500,6 @@ static void merge_name(const char *remote, struct strbuf *msg)
{
struct commit *remote_head;
struct object_id branch_head;
- struct strbuf buf = STRBUF_INIT;
struct strbuf bname = STRBUF_INIT;
struct merge_remote_desc *desc;
const char *ptr;
@@ -590,7 +587,6 @@ static void merge_name(const char *remote, struct strbuf *msg)
oid_to_hex(&remote_head->object.oid), remote);
cleanup:
free(found_ref);
- strbuf_release(&buf);
strbuf_release(&bname);
}
diff --git a/contrib/coccinelle/tests/unused.c b/contrib/coccinelle/tests/unused.c
new file mode 100644
index 00000000000..495ae58ccf1
--- /dev/null
+++ b/contrib/coccinelle/tests/unused.c
@@ -0,0 +1,55 @@
+void test_strbuf(void)
+{
+ struct strbuf sb1 = STRBUF_INIT;
+ struct strbuf sb2 = STRBUF_INIT;
+ struct strbuf sb3 = STRBUF_INIT;
+ struct strbuf sb4 = STRBUF_INIT;
+ struct strbuf sb5;
+ struct strbuf sb6 = { 0 };
+ struct strbuf sb7 = STRBUF_INIT;
+ struct strbuf sb8 = STRBUF_INIT;
+ struct strbuf *sp1;
+ struct strbuf *sp2;
+ struct strbuf *sp3;
+ struct strbuf *sp4 = xmalloc(sizeof(struct strbuf));
+ struct strbuf *sp5 = xmalloc(sizeof(struct strbuf));
+ struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
+ struct strbuf *sp7;
+
+ strbuf_init(&sb5, 0);
+ strbuf_init(sp1, 0);
+ strbuf_init(sp2, 0);
+ strbuf_init(sp3, 0);
+ strbuf_init(sp4, 0);
+ strbuf_init(sp5, 0);
+ strbuf_init(sp6, 0);
+ strbuf_init(sp7, 0);
+ sp7 = xmalloc(sizeof(struct strbuf));
+
+ use_before(&sb3);
+ use_as_str("%s", sb7.buf);
+ use_as_str("%s", sp1->buf);
+ use_as_str("%s", sp6->buf);
+ pass_pp(&sp3);
+
+ strbuf_release(&sb1);
+ strbuf_reset(&sb2);
+ strbuf_release(&sb3);
+ strbuf_release(&sb4);
+ strbuf_release(&sb5);
+ strbuf_release(&sb6);
+ strbuf_release(&sb7);
+ strbuf_release(sp1);
+ strbuf_release(sp2);
+ strbuf_release(sp3);
+ strbuf_release(sp4);
+ strbuf_release(sp5);
+ strbuf_release(sp6);
+ strbuf_release(sp7);
+
+ use_after(&sb4);
+
+ if (when_strict())
+ return;
+ strbuf_release(&sb8);
+}
diff --git a/contrib/coccinelle/tests/unused.res b/contrib/coccinelle/tests/unused.res
new file mode 100644
index 00000000000..b3b71053ed6
--- /dev/null
+++ b/contrib/coccinelle/tests/unused.res
@@ -0,0 +1,30 @@
+void test_strbuf(void)
+{
+ struct strbuf sb3 = STRBUF_INIT;
+ struct strbuf sb4 = STRBUF_INIT;
+ struct strbuf sb7 = STRBUF_INIT;
+ struct strbuf *sp1;
+ struct strbuf *sp3;
+ struct strbuf *sp6 = xmalloc(sizeof(struct strbuf));
+ strbuf_init(sp1, 0);
+ strbuf_init(sp3, 0);
+ strbuf_init(sp6, 0);
+
+ use_before(&sb3);
+ use_as_str("%s", sb7.buf);
+ use_as_str("%s", sp1->buf);
+ use_as_str("%s", sp6->buf);
+ pass_pp(&sp3);
+
+ strbuf_release(&sb3);
+ strbuf_release(&sb4);
+ strbuf_release(&sb7);
+ strbuf_release(sp1);
+ strbuf_release(sp3);
+ strbuf_release(sp6);
+
+ use_after(&sb4);
+
+ if (when_strict())
+ return;
+}
diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
new file mode 100644
index 00000000000..56530498d17
--- /dev/null
+++ b/contrib/coccinelle/unused.cocci
@@ -0,0 +1,32 @@
+// This rule finds sequences of "unused" declerations and uses of a
+// variable, where "unused" is defined to include only calling the
+// equivalent of alloc, init & free functions on the variable.
+@@
+type T;
+identifier I;
+constant INIT_MACRO =~ "^STRBUF_INIT$";
+identifier MALLOC1 =~ "^x?[mc]alloc$";
+identifier INIT_CALL1 =~ "^strbuf_init$";
+identifier REL1 =~ "^strbuf_(release|reset)$";
+@@
+
+(
+- T I;
+|
+- T I = { 0 };
+|
+- T I = INIT_MACRO;
+|
+- T I = MALLOC1(...);
+)
+
+<... when != \( I \| &I \)
+(
+- \( INIT_CALL1 \)( \( I \| &I \), ...);
+|
+- I = MALLOC1(...);
+)
+...>
+
+- \( REL1 \)( \( &I \| I \) );
+ ... when != \( I \| &I \)
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 28176914e57..97e71fe19cd 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -687,7 +687,7 @@ static int cmd_diagnose(int argc, const char **argv)
int stdout_fd = -1, archiver_fd = -1;
time_t now = time(NULL);
struct tm tm;
- struct strbuf path = STRBUF_INIT, buf = STRBUF_INIT;
+ struct strbuf buf = STRBUF_INIT;
int res = 0;
argc = parse_options(argc, argv, NULL, options,
@@ -779,7 +779,6 @@ static int cmd_diagnose(int argc, const char **argv)
free(argv_copy);
strvec_clear(&archiver_args);
strbuf_release(&zip_path);
- strbuf_release(&path);
strbuf_release(&buf);
return res;
diff --git a/diff.c b/diff.c
index e71cf758861..d4290615aaa 100644
--- a/diff.c
+++ b/diff.c
@@ -1289,7 +1289,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
{
static const char *nneof = " No newline at end of file\n";
const char *context, *reset, *set, *set_sign, *meta, *fraginfo;
- struct strbuf sb = STRBUF_INIT;
enum diff_symbol s = eds->s;
const char *line = eds->line;
@@ -1521,7 +1520,6 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
default:
BUG("unknown diff symbol");
}
- strbuf_release(&sb);
}
static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,
--
2.37.0.913.g50625c3f077
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 6/6] cocci: generalize "unused" rule to cover more than "strbuf"
2022-07-05 13:46 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2022-07-05 13:46 ` [PATCH v4 5/6] cocci: add and apply a rule to find "unused" strbufs Ævar Arnfjörð Bjarmason
@ 2022-07-05 13:47 ` Ævar Arnfjörð Bjarmason
2022-07-06 19:30 ` [PATCH v4 0/6] add and apply a rule to find "unused" init+free Junio C Hamano
2022-07-11 9:41 ` Jeff King
7 siblings, 0 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-05 13:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff King, Elijah Newren,
Eric Sunshine, Ævar Arnfjörð Bjarmason
Generalize the newly added "unused.cocci" rule to find more than just
"struct strbuf", let's have it find the same unused patterns for
"struct string_list", as well as other code that uses
similar-looking *_{release,clear,free}() and {release,clear,free}_*()
functions.
We're intentionally loose in accepting e.g. a "strbuf_init(&sb)"
followed by a "string_list_clear(&sb, 0)". It's assumed that the
compiler will catch any such invalid code, i.e. that our
constructors/destructors don't take a "void *".
See [1] for example of code that would be covered by the
"get_worktrees()" part of this rule. We'd still need work that the
series is based on (we were passing "worktrees" to a function), but
could now do the change in [1] automatically.
1. https://lore.kernel.org/git/Yq6eJFUPPTv%2Fzc0o@coredump.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/repack.c | 2 --
contrib/coccinelle/tests/unused.c | 27 +++++++++++++++++++++++++++
contrib/coccinelle/tests/unused.res | 15 +++++++++++++++
contrib/coccinelle/unused.cocci | 19 +++++++++++++++----
4 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 4a7ae4cf489..482b66f57d6 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -727,7 +727,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
struct child_process cmd = CHILD_PROCESS_INIT;
struct string_list_item *item;
struct string_list names = STRING_LIST_INIT_DUP;
- struct string_list rollback = STRING_LIST_INIT_NODUP;
struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;
struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
struct pack_geometry *geometry = NULL;
@@ -1117,7 +1116,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
}
string_list_clear(&names, 0);
- string_list_clear(&rollback, 0);
string_list_clear(&existing_nonkept_packs, 0);
string_list_clear(&existing_kept_packs, 0);
clear_pack_geometry(geometry);
diff --git a/contrib/coccinelle/tests/unused.c b/contrib/coccinelle/tests/unused.c
index 495ae58ccf1..8294d734ba4 100644
--- a/contrib/coccinelle/tests/unused.c
+++ b/contrib/coccinelle/tests/unused.c
@@ -53,3 +53,30 @@ void test_strbuf(void)
return;
strbuf_release(&sb8);
}
+
+void test_other(void)
+{
+ struct string_list l = STRING_LIST_INIT_DUP;
+ struct strbuf sb = STRBUF_INIT;
+
+ string_list_clear(&l, 0);
+ string_list_clear(&sb, 0);
+}
+
+void test_worktrees(void)
+{
+ struct worktree **w1 = get_worktrees();
+ struct worktree **w2 = get_worktrees();
+ struct worktree **w3;
+ struct worktree **w4;
+
+ w3 = get_worktrees();
+ w4 = get_worktrees();
+
+ use_it(w4);
+
+ free_worktrees(w1);
+ free_worktrees(w2);
+ free_worktrees(w3);
+ free_worktrees(w4);
+}
diff --git a/contrib/coccinelle/tests/unused.res b/contrib/coccinelle/tests/unused.res
index b3b71053ed6..6d3e745683c 100644
--- a/contrib/coccinelle/tests/unused.res
+++ b/contrib/coccinelle/tests/unused.res
@@ -28,3 +28,18 @@ void test_strbuf(void)
if (when_strict())
return;
}
+
+void test_other(void)
+{
+}
+
+void test_worktrees(void)
+{
+ struct worktree **w4;
+
+ w4 = get_worktrees();
+
+ use_it(w4);
+
+ free_worktrees(w4);
+}
diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci
index 56530498d17..d84046f82ea 100644
--- a/contrib/coccinelle/unused.cocci
+++ b/contrib/coccinelle/unused.cocci
@@ -4,10 +4,13 @@
@@
type T;
identifier I;
-constant INIT_MACRO =~ "^STRBUF_INIT$";
+// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)
+constant INIT_MACRO =~ "_INIT";
identifier MALLOC1 =~ "^x?[mc]alloc$";
-identifier INIT_CALL1 =~ "^strbuf_init$";
-identifier REL1 =~ "^strbuf_(release|reset)$";
+identifier INIT_ASSIGN1 =~ "^get_worktrees$";
+identifier INIT_CALL1 =~ "^[a-z_]*_init$";
+identifier REL1 =~ "^[a-z_]*_(release|reset|clear|free)$";
+identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
@@
(
@@ -18,15 +21,23 @@ identifier REL1 =~ "^strbuf_(release|reset)$";
- T I = INIT_MACRO;
|
- T I = MALLOC1(...);
+|
+- T I = INIT_ASSIGN1(...);
)
<... when != \( I \| &I \)
(
- \( INIT_CALL1 \)( \( I \| &I \), ...);
|
+- I = \( INIT_ASSIGN1 \)(...);
+|
- I = MALLOC1(...);
)
...>
-- \( REL1 \)( \( &I \| I \) );
+(
+- \( REL1 \| REL2 \)( \( I \| &I \), ...);
+|
+- \( REL1 \| REL2 \)( \( &I \| I \) );
+)
... when != \( I \| &I \)
--
2.37.0.913.g50625c3f077
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 0/6] add and apply a rule to find "unused" init+free
2022-07-05 13:46 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2022-07-05 13:47 ` [PATCH v4 6/6] cocci: generalize "unused" rule to cover more than "strbuf" Ævar Arnfjörð Bjarmason
@ 2022-07-06 19:30 ` Junio C Hamano
2022-07-11 9:41 ` Jeff King
7 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2022-07-06 19:30 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Derrick Stolee, Jeff King, Elijah Newren, Eric Sunshine
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> builtin/merge.c | 4 --
This finds and re-fixes what has already been fixed in another
topic, which is unfortunate, but it is minor. If this topic takes
longer before en/merge-restore-to-pristine graduates, it may want to
get rerolled to avoid duplication, but for now I'll queue the series
as-is.
Thanks.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 0/6] add and apply a rule to find "unused" init+free
2022-07-05 13:46 ` [PATCH v4 0/6] " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2022-07-06 19:30 ` [PATCH v4 0/6] add and apply a rule to find "unused" init+free Junio C Hamano
@ 2022-07-11 9:41 ` Jeff King
2022-07-11 10:54 ` Ævar Arnfjörð Bjarmason
7 siblings, 1 reply; 24+ messages in thread
From: Jeff King @ 2022-07-11 9:41 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Elijah Newren, Eric Sunshine
On Tue, Jul 05, 2022 at 03:46:54PM +0200, Ævar Arnfjörð Bjarmason wrote:
> This series adds a coccinelle rule to find and remove code where the
> only reference to a variable in a given function is to malloc() &
> free() it, where "malloc" and "free" also match
> "strbuf_init/strbuf_release", and then later in the series anything
> that looks like a init/free pattern.
As before, I'm perfectly happy with the actual transformations here.
> Changes since v3[1]
>
> * Add a "coccicheck-test" target in an early and new patch, the
> structure mirrors that of coccinelle.git's own tests. As the
> diffstat shows we have a *.c and *.res file which is C code
> before/after a *.cocci rule is applied.
I have mixed feelings on this. I'm not opposed per se, and I even like
the fact that it provides examples of what we expect a rule to do. But I
worry about a world where the cost of using coccinelle goes up because
now transformations need to have tests along with them. The idea of
cocci is to make your life a little simpler and more efficient when
fixing up the code base (as opposed to perl, etc). But if it comes with
a bunch of boilerplate tasks, that might no longer be the case. I'm
already often on the fence about using cocci just because of the hours
I've sunk into debugging and puzzling things out.
I dunno. Maybe the bar is higher for stuff like this that we expect to
continue to find problems as time goes on (because it is not "here is a
transition", but "this is a common and easy mistake to make").
So again, I'm not really opposed to this patch in particular, but I want
to express caution at the direction we might be going, and at applying
new rules over-zealously.
> * We now catch init/reset patterns as well as init/release fully
> (i.e. for the "struct strbuf" early on)
Yeah, that makes sense.
-Peff
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 0/6] add and apply a rule to find "unused" init+free
2022-07-11 9:41 ` Jeff King
@ 2022-07-11 10:54 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-11 10:54 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Derrick Stolee, Elijah Newren, Eric Sunshine
On Mon, Jul 11 2022, Jeff King wrote:
> On Tue, Jul 05, 2022 at 03:46:54PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> This series adds a coccinelle rule to find and remove code where the
>> only reference to a variable in a given function is to malloc() &
>> free() it, where "malloc" and "free" also match
>> "strbuf_init/strbuf_release", and then later in the series anything
>> that looks like a init/free pattern.
>
> As before, I'm perfectly happy with the actual transformations here.
Thanks!
>> Changes since v3[1]
>>
>> * Add a "coccicheck-test" target in an early and new patch, the
>> structure mirrors that of coccinelle.git's own tests. As the
>> diffstat shows we have a *.c and *.res file which is C code
>> before/after a *.cocci rule is applied.
>
> I have mixed feelings on this. I'm not opposed per se, and I even like
> the fact that it provides examples of what we expect a rule to do. But I
> worry about a world where the cost of using coccinelle goes up because
> now transformations need to have tests along with them. The idea of
> cocci is to make your life a little simpler and more efficient when
> fixing up the code base (as opposed to perl, etc).
That's fair, and I suppose we could add some really complex tests that
e.g. include the original headers.
But right now these just work on stand-alone *.c files with no includes,
and the runtime is thus trivial. So I thought it was OK to run it along
with the main job.
> But if it comes with a bunch of boilerplate tasks, that might no
> longer be the case. I'm already often on the fence about using cocci
> just because of the hours I've sunk into debugging and puzzling things
> out.
>
> I dunno. Maybe the bar is higher for stuff like this that we expect to
> continue to find problems as time goes on (because it is not "here is a
> transition", but "this is a common and easy mistake to make").
>
> So again, I'm not really opposed to this patch in particular, but I want
> to express caution at the direction we might be going, and at applying
> new rules over-zealously.
Yeah, we definitely need to be more careful with "ongoing" rules like
these, but hopefully between the tests & lack of false positives this
one looks sane...
>> * We now catch init/reset patterns as well as init/release fully
>> (i.e. for the "struct strbuf" early on)
>
> Yeah, that makes sense.
>
> -Peff
^ permalink raw reply [flat|nested] 24+ messages in thread