git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Taylor Blau" <me@ttaylorr.com>, "René Scharfe" <l.s.r@web.de>,
	git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH] git-compat-util.h: introduce CALLOC(x)
Date: Tue, 06 Dec 2022 02:58:53 +0100	[thread overview]
Message-ID: <221206.86v8mpcmnt.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <Y46fHhD3xionHgPE@coredump.intra.peff.net>


On Mon, Dec 05 2022, Jeff King wrote:

> On Tue, Dec 06, 2022 at 12:12:32AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> 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(),
>> and REALLOC() and REALLOC_ARRAY()?
>
> FWIW, I would be happy to see all of those (minus REALLOC(), as there is
> not really any point in growing or shrinking something with a fixed
> size).
>
> The biggest argument against them that I can see is that:
>
>   struct foo *x = malloc(sizeof(*x));
>
> is idiomatic C that newcomers to the project will easily understand,
> and:
>
>   struct foo *x;
>   ALLOC(x);
>
> is not. But it feels like we already crossed that bridge with
> ALLOC_ARRAY(), etc.

This is probably too ugly to exist, but if we are going to have more
variants maybe one that would allow use within declarations would be
better, e.g.:

	
	diff --git a/attr.c b/attr.c
	index 42ad6de8c7c..43ade426e57 100644
	--- a/attr.c
	+++ b/attr.c
	@@ -666,11 +666,10 @@ static void handle_attr_line(struct attr_stack *res,
	 
	 static struct attr_stack *read_attr_from_array(const char **list)
	 {
	-	struct attr_stack *res;
	+	struct attr_stack *INIT_CALLOC_ARRAY(res, 1);
	 	const char *line;
	 	int lineno = 0;
	 
	-	CALLOC_ARRAY(res, 1);
	 	while ((line = *(list++)) != NULL)
	 		handle_attr_line(res, line, "[builtin]", ++lineno,
	 				 READ_ATTR_MACRO_OK);
	diff --git a/git-compat-util.h b/git-compat-util.h
	index a76d0526f79..932d0907f3f 100644
	--- a/git-compat-util.h
	+++ b/git-compat-util.h
	@@ -1089,6 +1089,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);
	 
	 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
	 #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))
	+#define INIT_CALLOC_ARRAY(x, alloc) x = xcalloc((alloc), sizeof(*(x)))
	 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))
	 
	 #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \

I suspect (but haven't checked) that it might get us into the same
parsing trouble as your UNUSED(name) macro.

Or maybe:
	
	diff --git a/attr.c b/attr.c
	index 42ad6de8c7c..c3cb5c98bbf 100644
	--- a/attr.c
	+++ b/attr.c
	@@ -669 +669 @@ static struct attr_stack *read_attr_from_array(const char **list)
	-	struct attr_stack *res;
	+	INIT_CALLOC_ARRAY(struct attr_stack *, res, 1);
	@@ -673 +672,0 @@ static struct attr_stack *read_attr_from_array(const char **list)
	-	CALLOC_ARRAY(res, 1);
	diff --git a/git-compat-util.h b/git-compat-util.h
	index a76d0526f79..fd9af571dc9 100644
	--- a/git-compat-util.h
	+++ b/git-compat-util.h
	@@ -1091,0 +1092 @@ int xstrncmpz(const char *s, const char *t, size_t len);
	+#define INIT_CALLOC_ARRAY(d, x, alloc) d x = xcalloc((alloc), sizeof(*(x)))
	
I think it's also worth considering just having the "normal C' versions,
but we could transform anything that doesn't look lik ea narrow
whitelist of patterns into an error with coccinelle.

I.e. if we're paranoid about "v" v.s. "*var" in the "sizeof" we could
also check & tranform that with coccinelle...

  reply	other threads:[~2022-12-06  2:05 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 [this message]
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:
  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=221206.86v8mpcmnt.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    /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).