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

On Tue, Dec 06, 2022 at 12:12:32AM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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.

Agreed, but I do tend to consider this patch too-large to apply right

Whether or not 2k lines of diff is review-able or not (and FWIW, I think
that your recent Coccinelle-generated patches were quite easy to review
pretty quickly), I think there's a bigger question of whether or not we
want to make such a sweeping, tree-wide change. And assuming the answer
to that is "yes", there's is also a question of timing. Proposing it
towards the middle or end of a release cycle seems in bad taste.

But I think splitting this discussion up into, "should we introduce
something like CALLOC() into our style conventions?" and "do we want to
apply this everywhere?" is worth doing. The former doesn't seem to take
a ton of time away from polishing the release candidates, and the latter
can be done only after the former has settled.

So the decision to make this a *.pending.cocci rule was definitely
intentional in this case.

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

Thanks for mentioning.

> 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:

See above.

> 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 think the point is that it's just weird. Yes, an array of just a
single element on the heap is indistinguishable from asking malloc() to
give me the same bytes and then memset() them myself, just as it's
indistinguishable from calloc()'ing the right number of bytes for a
single structure to begin with.

But whether or not the two are indistinguishable doesn't mean that it
makes intuitive sense, and that is the goal of CALLOC().


  parent reply	other threads:[~2022-12-07  2:35 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
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 [this message]
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 \
    --in-reply-to=Y4/7txDTWK5nGPDu@nand.local \ \ \ \ \ \ \

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