git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <derrickstolee@github.com>,
	Elijah Newren <newren@gmail.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v4 0/6] add and apply a rule to find "unused" init+free
Date: Mon, 11 Jul 2022 05:41:00 -0400	[thread overview]
Message-ID: <YsvwLIfjZffUWOP/@coredump.intra.peff.net> (raw)
In-Reply-To: <cover-v4-0.6-00000000000-20220705T134033Z-avarab@gmail.com>

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

  parent reply	other threads:[~2022-07-11 10:31 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-20 11:57 [PATCH] cocci: add and apply a rule to find "unused" variables Ævar Arnfjörð Bjarmason
2022-06-21 22:44 ` [PATCH v2 0/2] add and apply a rule to find "unused" init+free Ævar Arnfjörð Bjarmason
2022-06-21 22:44   ` [PATCH v2 1/2] cocci: add and apply a rule to find "unused" variables Ævar Arnfjörð Bjarmason
2022-06-22 16:02     ` Junio C Hamano
2022-06-21 22:44   ` [PATCH v2 2/2] cocci: remove "when strict" from unused.cocci Ævar Arnfjörð Bjarmason
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 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
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
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     ` [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
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       ` [PATCH v4 3/6] cocci: add a "coccicheck-test" target and test *.cocci rules Æ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
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       ` [PATCH v4 6/6] cocci: generalize "unused" rule to cover more than "strbuf" Æ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 [this message]
2022-07-11 10:54         ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YsvwLIfjZffUWOP/@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).