diff mbox series

[v3,1/4] cocci: add and apply a rule to find "unused" strbufs

Message ID patch-v3-1.4-49e9ccb5819-20220701T102506Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series add and apply a rule to find "unused" init+free | expand

Commit Message

Ævar Arnfjörð Bjarmason July 1, 2022, 10:30 a.m. UTC
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

Comments

Jeff King July 1, 2022, 6:04 p.m. UTC | #1
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
Eric Sunshine July 1, 2022, 7:55 p.m. UTC | #2
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/
diff mbox series

Patch

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(&note);
-	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,