mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <>
To: Taylor Blau <>
Cc: "René Scharfe" <>,, "Jeff King" <>,
	"Junio C Hamano" <>
Subject: Re: [PATCH] git-compat-util.h: introduce CALLOC(x)
Date: Tue, 06 Dec 2022 00:12:32 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <Y45yaYV3xFB/xR2G@nand.local>

On Mon, Dec 05 2022, Taylor Blau wrote:

> On Mon, Dec 05, 2022 at 10:01:11PM +0100, René Scharfe wrote:
>> This rule would turn this code:
>> 	struct foo *bar = xcalloc(1, sizeof(*bar));
>> 	int i;
>> ... into:
>> 	struct foo *bar;
>> 	CALLOC(bar);
>> 	int i;
>> ... which violates the coding guideline to not mix declarations and
>> statements (-Wdeclaration-after-statement).
> Yeah, I was wondering about this myself when I wrote this part of the
> Coccinelle patch.
> Is there an intelligent way to tell it to put the first statement after
> all declarations? I couldn't find anything after a quick scan of the
> documentation nor our own patches.

We can get that working, but I think it's good to establish some ground
rules first:

First, when you add *.pending.cocci rules they shouldn't be pseudocode,
but things that are too large to apply right now. I think my recent
041df69edd3 (Merge branch 'ab/fewer-the-index-macros', 2022-11-28) is a
good example.

In this case if I apply the proposed patch and do:

	make contrib/coccinelle/xcalloc.pending.cocci.patch &&
	patch -p1 <contrib/coccinelle/xcalloc.pending.cocci.patch &&

We have a deluge of syntax errors, not just the warnings René
notes. This needs to be fixed, the *.pending.cocci must actually work on
our code.

(We should probably modify the "static-analysis" to actually apply the
result of these *.pending patches and compile with those, but until then
we can manually test them).

Second, we have test support for rules now, see
contrib/coccinelle/tests/. You just need to create a
contrib/coccinelle/tests/xcalloc.pending.c, and have the expected
post-image in contrib/coccinelle/tests/xcalloc.pending.res. Please add
one of those. We don't have them for existing rules, but it really helps
to assert & review new rules.

The various edge cases that your current *.cocci doesn't compile on
etc. should go into that test file.

Third, the resulting patch is currently ~2k lines. Can we really not
make it non-pending with some whitelisting/gblacklisting. E.g. see this
out-of-tree patch for an example of opting out certain functions:

I think that's preferable to having some *.pending.cocci we may or may
not get to. I think you can also probably write a working rule for this
to incrementally target certain subsets of these, and we could just
mass-convert some stuff already (if it doesn't conflict with in-flight

Fourth, I must say I'm kind of negative on the proposed change. I.e. the
foot-gun is definitely worth solving, but why not just create a
coccinelle rule to narrowly address that? In that case we can presumably
start with it non-pending, as we don't have that many of them.

On the notion that we need to special-case:

	- CALLOC_ARRAY(ptr, 1)
	+ CALLOC(ptr)

Because an array of one is "not an array" I don't really buy it. The
calloc() interface itself exposes that, so I don't see why we'd consider
those separate on the API level for these wrapper macros.

I.e. I think the point of the macro should be to provide the appropiate
sizeof() for you, not to be opinionated that nmemb=1 should be
special-cased.I think it's probably not worth the churn to make that

But if we *are* doing that then surely we should provide the full set of
functions. I.e. ALLOC() and ALLOC_ARRAY(), CALLOC() and CALLOC_ARRAY(),

  reply	other threads:[~2022-12-05 23:40 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-05 18:54 [PATCH] git-compat-util.h: introduce CALLOC(x) Taylor Blau
2022-12-05 21:01 ` René Scharfe
2022-12-05 22:36   ` Taylor Blau
2022-12-05 23:12     ` Ævar Arnfjörð Bjarmason [this message]
2022-12-06  1:47       ` Jeff King
2022-12-06  1:58         ` Ævar Arnfjörð Bjarmason
2022-12-07  6:02           ` Jeff King
2022-12-07  2:36         ` Taylor Blau
2022-12-07  2:34       ` Taylor Blau
2022-12-07  3:17         ` Ævar Arnfjörð Bjarmason
2022-12-06  1:43     ` Jeff King
2022-12-07  2:29       ` Taylor Blau
2022-12-07  3:51         ` Ævar Arnfjörð Bjarmason
2022-12-05 23:57 ` Junio C Hamano
2022-12-06  0:29   ` Taylor Blau
2022-12-06  1:21     ` Jeff King
2022-12-06  1:35       ` Junio C Hamano
2022-12-07  2:38         ` Taylor Blau
2022-12-07  6:08           ` Jeff King
2022-12-06  2:12     ` Ævar Arnfjörð Bjarmason
2022-12-07  6:06       ` Jeff King

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:

  List information:

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

  git send-email \ \ \ \ \ \ \ \

* 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

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).