{"thread":{"id":"58897","subject":"[PATCH] git-compat-util.h: introduce CALLOC(x)","startedAt":"2022-12-05T18:54:49Z","lastAt":"2022-12-07T06:08:15Z","messageCount":21,"participants":["Taylor Blau","René Scharfe","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"468540","messageId":"6694c52b38674859eb0390c7f62da1209a8d8ec3.1670266373.git.me@ttaylorr.com","threadId":"58897","inReplyTo":null,"subject":"[PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-12-05T18:54:38Z","receivedAt":"2022-12-05T18:54:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When zero-initializing a struct without the use of static initializers\nlike \"{ 0 }\", it is common to write one of:\n\n T *ptr = xcalloc(1, sizeof(T));\n T *ptr = xcalloc(1, sizeof(*ptr));\n\nThese correctly initialize \"*ptr\" to the all-zeros value, but have a\ncouple of drawbacks. Notably, both initializations are verbose, but the\nformer is a foot-gun. If \"*ptr\"'s type changes to something other than\n\"T\", the programmer has to remember to update not only the type of the\nvariable, but the right-hand side of its initialization, too.\n\nIn git.git, it is sometimes common to write something like:\n\n T *ptr;\n CALLOC_ARRAY(ptr, 1);\n\n...but that is confusing, since we're not initializing an array. Rather,\nwe're using the CALLOC_ARRAY() macro to pretend like we are. But since\nthe array length is 1, the effect is zero initializing a single element.\n\nIntroduce a new variant, CALLOC(x), which initializes \"x\" to the\nall-zeros value, without exposing the confusing use of CALLOC_ARRAY(),\nor the foot-guns available when using xcalloc() directly.\n\nWrite a Coccinelle patch which codifies these rules, but mark it as\n\"pending\" since the resulting diff is too large to include in a single\npatch:\n\n $ git apply .build/contrib/coccinelle/xcalloc.pending.cocci.patch\n $ git diff --shortstat\n 89 files changed, 221 insertions(+), 178 deletions(-)\n\nSigned-off-by: Taylor Blau \n---\nThis is a follow-up on [1], where introducing CALLOC(x) as the preferred\nalternative to CALLOC_ARRAY(x, 1) was first suggested.\n\nThe patch is straightforward, and I am pretty sure that the Coccinelle\nrules are right, too ;-). But as a first-time Coccinelle user, any extra\nscrutiny on those bits would be appreciated.\n\nThe main point of discussion I think is whether we should consider\nadopting this rule. I am biased, of course, but I think that we should.\n\nIn any case, we should focus our efforts on polishing v2.39.0, but I\nwanted to send this out to the list before I forgot about it.\n\n[1]: https://lore.kernel.org/git/Y1MrXoobkVKngYL1@coredump.intra.peff.net/\n\n contrib/coccinelle/xcalloc.pending.cocci | 21 +++++++++++++++++++++\n git-compat-util.h | 8 ++++++++\n 2 files changed, 29 insertions(+)\n create mode 100644 contrib/coccinelle/xcalloc.pending.cocci\n\ndiff --git a/contrib/coccinelle/xcalloc.pending.cocci b/contrib/coccinelle/xcalloc.pending.cocci\nnew file mode 100644\nindex 0000000000..83e4ca1a68\n--- /dev/null\n+++ b/contrib/coccinelle/xcalloc.pending.cocci\n@@ -0,0 +1,21 @@\n+@@\n+type T;\n+T *ptr;\n+@@\n+- ptr = xcalloc(1, \\( sizeof(T) \\| sizeof(*ptr) \\) )\n++ CALLOC(ptr)\n+\n+@@\n+type T;\n+identifier ptr;\n+@@\n+- T ptr = xcalloc(1, \\( sizeof(T) \\| sizeof(*ptr) \\) );\n++ T ptr;\n++ CALLOC(ptr);\n+\n+@@\n+type T;\n+T *ptr;\n+@@\n+- CALLOC_ARRAY(ptr, 1)\n++ CALLOC(ptr)\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a76d0526f7..827e5be89c 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1107,6 +1107,14 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)\n \t\tmemmove(dst, src, st_mult(size, n));\n }\n\n+/*\n+ * Like CALLOC_ARRAY, but the argument is treated as a pointer to a\n+ * single struct.\n+ *\n+ * Preferable over xcalloc(1, sizeof(...)), or CALLOC_ARRAY(..., 1).\n+ */\n+#define CALLOC(x) (x) = CALLOC_ARRAY((x), 1)\n+\n /*\n * These functions help you allocate structs with flex arrays, and copy\n * the data directly into the array. For example, if you had:\n--\n2.38.0.16.g393fd4c6db\n"},{"id":"468544","messageId":"a8e33b1e-1056-5f75-55b5-65c0bceef3ca@web.de","threadId":"58897","inReplyTo":"6694c52b38674859eb0390c7f62da1209a8d8ec3.1670266373.git.me@ttaylorr.com","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-05T21:01:11Z","receivedAt":"2022-12-05T21:03:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.12.22 um 19:54 schrieb Taylor Blau:\n> When zero-initializing a struct without the use of static initializers\n> like \"{ 0 }\", it is common to write one of:\n>\n> T *ptr = xcalloc(1, sizeof(T));\n> T *ptr = xcalloc(1, sizeof(*ptr));\n>\n> These correctly initialize \"*ptr\" to the all-zeros value, but have a\n> couple of drawbacks. Notably, both initializations are verbose, but the\n> former is a foot-gun. If \"*ptr\"'s type changes to something other than\n> \"T\", the programmer has to remember to update not only the type of the\n> variable, but the right-hand side of its initialization, too.\n>\n> In git.git, it is sometimes common to write something like:\n>\n> T *ptr;\n> CALLOC_ARRAY(ptr, 1);\n>\n> ...but that is confusing, since we're not initializing an array. Rather,\n> we're using the CALLOC_ARRAY() macro to pretend like we are. But since\n> the array length is 1, the effect is zero initializing a single element.\n\nAn object and a single-element array of objects allocated on the heap are\nindistinguishable. In that sense there is no confusion -- we are indeed\nallocating a single-element array. But if the intent is to only get one\nthing then having to fill in 1 in the bulk order form is a bit tedious,\nespecially since this is the most common kind of request. A shortcut for\nthe frequent case makes sense.\n\n> Introduce a new variant, CALLOC(x), which initializes \"x\" to the\n> all-zeros value, without exposing the confusing use of CALLOC_ARRAY(),\n> or the foot-guns available when using xcalloc() directly.\n\nAFAIK the first \"c\" in \"calloc\" is for \"continuous\", not \"zeroed\". A\nsingle object is always contiguous, so the \"C\" in \"CALLOC\" is\ntautologic if read in that way. It fits our naming scheme for\nALLOC_ARRAY and CALLOC_ARRAY, though, so that might not be much of a\nproblem.\n\nAnd there are lots of occurrences of xmalloc(sizeof(T)) and\nxmalloc(sizeof(*ptr)) that would benefit from the automatic size\ninference of ALLOC_ARRAY -- an ALLOC macro would complement\nCALLOC nicely.\n\n> Write a Coccinelle patch which codifies these rules, but mark it as\n> \"pending\" since the resulting diff is too large to include in a single\n> patch:\n>\n> $ git apply .build/contrib/coccinelle/xcalloc.pending.cocci.patch\n> $ git diff --shortstat\n> 89 files changed, 221 insertions(+), 178 deletions(-)\n>\n> Signed-off-by: Taylor Blau \n> ---\n> This is a follow-up on [1], where introducing CALLOC(x) as the preferred\n> alternative to CALLOC_ARRAY(x, 1) was first suggested.\n>\n> The patch is straightforward, and I am pretty sure that the Coccinelle\n> rules are right, too ;-). But as a first-time Coccinelle user, any extra\n> scrutiny on those bits would be appreciated.\n>\n> The main point of discussion I think is whether we should consider\n> adopting this rule. I am biased, of course, but I think that we should.\n>\n> In any case, we should focus our efforts on polishing v2.39.0, but I\n> wanted to send this out to the list before I forgot about it.\n>\n> [1]: https://lore.kernel.org/git/Y1MrXoobkVKngYL1@coredump.intra.peff.net/\n>\n> contrib/coccinelle/xcalloc.pending.cocci | 21 +++++++++++++++++++++\n> git-compat-util.h | 8 ++++++++\n> 2 files changed, 29 insertions(+)\n> create mode 100644 contrib/coccinelle/xcalloc.pending.cocci\n>\n> diff --git a/contrib/coccinelle/xcalloc.pending.cocci b/contrib/coccinelle/xcalloc.pending.cocci\n> new file mode 100644\n> index 0000000000..83e4ca1a68\n> --- /dev/null\n> +++ b/contrib/coccinelle/xcalloc.pending.cocci\n> @@ -0,0 +1,21 @@\n> +@@\n> +type T;\n> +T *ptr;\n> +@@\n> +- ptr = xcalloc(1, \\( sizeof(T) \\| sizeof(*ptr) \\) )\n> ++ CALLOC(ptr)\n> +\n> +@@\n> +type T;\n> +identifier ptr;\n> +@@\n> +- T ptr = xcalloc(1, \\( sizeof(T) \\| sizeof(*ptr) \\) );\n> ++ T ptr;\n> ++ CALLOC(ptr);\n\nThis rule would turn this code:\n\n\tstruct foo *bar = xcalloc(1, sizeof(*bar));\n\tint i;\n\n... into:\n\n\tstruct foo *bar;\n\tCALLOC(bar);\n\tint i;\n\n... which violates the coding guideline to not mix declarations and\nstatements (-Wdeclaration-after-statement).\n\n> +\n> +@@\n> +type T;\n> +T *ptr;\n> +@@\n> +- CALLOC_ARRAY(ptr, 1)\n> ++ CALLOC(ptr)\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index a76d0526f7..827e5be89c 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -1107,6 +1107,14 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)\n> \t\tmemmove(dst, src, st_mult(size, n));\n> }\n>\n> +/*\n> + * Like CALLOC_ARRAY, but the argument is treated as a pointer to a\n> + * single struct.\n> + *\n> + * Preferable over xcalloc(1, sizeof(...)), or CALLOC_ARRAY(..., 1).\n> + */\n> +#define CALLOC(x) (x) = CALLOC_ARRAY((x), 1)\n> +\n> /*\n> * These functions help you allocate structs with flex arrays, and copy\n> * the data directly into the array. For example, if you had:\n> --\n> 2.38.0.16.g393fd4c6db\n\n"},{"id":"468550","messageId":"Y45yaYV3xFB/xR2G@nand.local","threadId":"58897","inReplyTo":"a8e33b1e-1056-5f75-55b5-65c0bceef3ca@web.de","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-12-05T22:36:25Z","receivedAt":"2022-12-05T22:36:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Dec 05, 2022 at 10:01:11PM +0100, René Scharfe wrote:\n> This rule would turn this code:\n>\n> \tstruct foo *bar = xcalloc(1, sizeof(*bar));\n> \tint i;\n>\n> ... into:\n>\n> \tstruct foo *bar;\n> \tCALLOC(bar);\n> \tint i;\n>\n> ... which violates the coding guideline to not mix declarations and\n> statements (-Wdeclaration-after-statement).\n\nYeah, I was wondering about this myself when I wrote this part of the\nCoccinelle patch.\n\nIs there an intelligent way to tell it to put the first statement after\nall declarations? I couldn't find anything after a quick scan of the\ndocumentation nor our own patches.\n\nThanks,\nTaylor\n"},{"id":"468560","messageId":"221206.868rjle7za.gmgdl@evledraar.gmail.com","threadId":"58897","inReplyTo":"Y45yaYV3xFB/xR2G@nand.local","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-05T23:12:32Z","receivedAt":"2022-12-05T23:39:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 05 2022, Taylor Blau wrote:\n\n> On Mon, Dec 05, 2022 at 10:01:11PM +0100, René Scharfe wrote:\n>> This rule would turn this code:\n>>\n>> \tstruct foo *bar = xcalloc(1, sizeof(*bar));\n>> \tint i;\n>>\n>> ... into:\n>>\n>> \tstruct foo *bar;\n>> \tCALLOC(bar);\n>> \tint i;\n>>\n>> ... which violates the coding guideline to not mix declarations and\n>> statements (-Wdeclaration-after-statement).\n>\n> Yeah, I was wondering about this myself when I wrote this part of the\n> Coccinelle patch.\n>\n> Is there an intelligent way to tell it to put the first statement after\n> all declarations? I couldn't find anything after a quick scan of the\n> documentation nor our own patches.\n\nWe can get that working, but I think it's good to establish some ground\nrules first:\n\nFirst, when you add *.pending.cocci rules they shouldn't be pseudocode,\nbut things that are too large to apply right now. I think my recent\n041df69edd3 (Merge branch 'ab/fewer-the-index-macros', 2022-11-28) is a\ngood example.\n\nIn this case if I apply the proposed patch and do:\n\n\tmake contrib/coccinelle/xcalloc.pending.cocci.patch &&\n\tpatch -p1 writes:\n\n> In git.git, it is sometimes common to write something like:\n>\n> T *ptr;\n> CALLOC_ARRAY(ptr, 1);\n>\n> ...but that is confusing, since we're not initializing an array.\n\nGiven that \"man calloc\" tells us that calloc takes two parameters,\n\n void *calloc(size_t nmemb, size_t size);\n\nI personally find CALLOC() that takes only a single parameter and is\ncapable only to allocate a single element array very much confusing.\n\nIt _might_ be arguable that the order of the parameters CALLOC_ARRAY\ntakes should have been reversed in that the number of elements in\nthe array should come first just like in calloc(), while the pointer\nthat is used to infer the size of an array element should come next,\nbut that is water under the bridge.\n\n"},{"id":"468565","messageId":"Y46M4oksPQkqwmTC@nand.local","threadId":"58897","inReplyTo":"xmqqedtdpfoe.fsf@gitster.g","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-12-06T00:29:22Z","receivedAt":"2022-12-06T00:29:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Dec 06, 2022 at 08:57:21AM +0900, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > In git.git, it is sometimes common to write something like:\n> >\n> > T *ptr;\n> > CALLOC_ARRAY(ptr, 1);\n> >\n> > ...but that is confusing, since we're not initializing an array.\n>\n> Given that \"man calloc\" tells us that calloc takes two parameters,\n>\n> void *calloc(size_t nmemb, size_t size);\n>\n> I personally find CALLOC() that takes only a single parameter and is\n> capable only to allocate a single element array very much confusing.\n\nHmm. I have always considered \"calloc\" a mental shorthand for \"zero\ninitialize some bytes on the heap\". It seemed like you were in favor of\nsuch a change in:\n\n https://lore.kernel.org/git/xmqq8rl8ivlb.fsf@gitster.g/\n\n...but it's entirely possible that I misread your message, in which case\nI would not be sad if you dropped this patch on the floor since I don't\nfeel that strongly about it.\n\nI'd be fine to call it CALLOC_ONE() or something, but I'm not sure at\nthat point if it's significantly better to write \"CALLOC_ONE(x)\" versus\n\"CALLOC_ARRAY(foo, 1)\"\n\n> It _might_ be arguable that the order of the parameters CALLOC_ARRAY\n> takes should have been reversed in that the number of elements in\n> the array should come first just like in calloc(), while the pointer\n> that is used to infer the size of an array element should come next,\n> but that is water under the bridge.\n\nYes, I agree that that would be better. But it would be frustrating to\nmake a tree-wide change of that magnitude at this point. So I agree it's\nwater under the bridge ;-).\n\n\nThanks,\nTaylor\n"},{"id":"468571","messageId":"Y46ZJUsyp8UW5rFW@coredump.intra.peff.net","threadId":"58897","inReplyTo":"Y46M4oksPQkqwmTC@nand.local","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-06T01:21:41Z","receivedAt":"2022-12-06T01:22:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 05, 2022 at 07:29:22PM -0500, Taylor Blau wrote:\n\n> On Tue, Dec 06, 2022 at 08:57:21AM +0900, Junio C Hamano wrote:\n> > Taylor Blau writes:\n> >\n> > > In git.git, it is sometimes common to write something like:\n> > >\n> > > T *ptr;\n> > > CALLOC_ARRAY(ptr, 1);\n> > >\n> > > ...but that is confusing, since we're not initializing an array.\n> >\n> > Given that \"man calloc\" tells us that calloc takes two parameters,\n> >\n> > void *calloc(size_t nmemb, size_t size);\n> >\n> > I personally find CALLOC() that takes only a single parameter and is\n> > capable only to allocate a single element array very much confusing.\n> \n> Hmm. I have always considered \"calloc\" a mental shorthand for \"zero\n> initialize some bytes on the heap\". It seemed like you were in favor of\n> such a change in:\n> \n> https://lore.kernel.org/git/xmqq8rl8ivlb.fsf@gitster.g/\n> \n> ...but it's entirely possible that I misread your message, in which case\n> I would not be sad if you dropped this patch on the floor since I don't\n> feel that strongly about it.\n\nI think the weird thing about calloc is that it does _two_ things\ncompared to malloc: zero-ing, and doing an implicit nmemb*size\nmultiplication.\n\nSo you can think of \"allocate this one element and zero it\" as \"calloc,\nbut don't multiply\" or as \"malloc, but zero\". Naming it CALLOC() is\nthinking of it as the former. If we think of it as the latter it could\nperhaps be MALLOCZ() or something. I don't know if that name is too\nsubtle or not. We have xmemdupz(), which is basically the same thing;\nit's only a zero-terminator, but that is because we are writing non-zero\nbytes in the rest of it. Mostly I'd worry that it is easy to glance past\nthe \"Z\".\n\n-Peff\n"},{"id":"468572","messageId":"xmqqv8mpnwjp.fsf@gitster.g","threadId":"58897","inReplyTo":"Y46ZJUsyp8UW5rFW@coredump.intra.peff.net","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-06T01:35:54Z","receivedAt":"2022-12-06T01:35:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So you can think of \"allocate this one element and zero it\" as \"calloc,\n> but don't multiply\" or as \"malloc, but zero\". Naming it CALLOC() is\n> thinking of it as the former. If we think of it as the latter it could\n> perhaps be MALLOCZ() or something. I don't know if that name is too\n> subtle or not. We have xmemdupz(), which is basically the same thing;\n> it's only a zero-terminator, but that is because we are writing non-zero\n> bytes in the rest of it. Mostly I'd worry that it is easy to glance past\n> the \"Z\".\n\nI think the name for the former would be CALLOC_ONE(), as I would\nrephrase it as \"calloc, but just one element\". I agree MALLOCZ()\nwould be fine for the other interpretation, and I do not have much\nproblem as much problem with the name as calling it CALLOC().\n\nThanks.\n\n"},{"id":"468576","messageId":"Y46eVnYrcOGAbUhi@coredump.intra.peff.net","threadId":"58897","inReplyTo":"Y45yaYV3xFB/xR2G@nand.local","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-06T01:43:50Z","receivedAt":"2022-12-06T01:43:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 05, 2022 at 05:36:25PM -0500, Taylor Blau wrote:\n\n> On Mon, Dec 05, 2022 at 10:01:11PM +0100, René Scharfe wrote:\n> > This rule would turn this code:\n> >\n> > \tstruct foo *bar = xcalloc(1, sizeof(*bar));\n> > \tint i;\n> >\n> > ... into:\n> >\n> > \tstruct foo *bar;\n> > \tCALLOC(bar);\n> > \tint i;\n> >\n> > ... which violates the coding guideline to not mix declarations and\n> > statements (-Wdeclaration-after-statement).\n> \n> Yeah, I was wondering about this myself when I wrote this part of the\n> Coccinelle patch.\n> \n> Is there an intelligent way to tell it to put the first statement after\n> all declarations? I couldn't find anything after a quick scan of the\n> documentation nor our own patches.\n\nIt feels like generating the code as above is not the end of the world.\nThe most valuable thing that coccinelle is doing here is _finding_ the\nlocation, and telling you \"it's supposed to be like this\". It is great\nwhen the \"this\" post-image is perfect and doesn't need further tweaking.\n\nBut if the compiler then reminds you \"hey, you need to go a bit further\nmanually\", that doesn't seem so bad. In other words, I would be happy to\nfollow that work flow if I introduced a bare xcalloc(). My only worry is\nthat somebody less experienced with the project (or with C) would get\nconfused.\n\n-Peff\n"},{"id":"468577","messageId":"Y46fHhD3xionHgPE@coredump.intra.peff.net","threadId":"58897","inReplyTo":"221206.868rjle7za.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-06T01:47:10Z","receivedAt":"2022-12-06T01:47:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 06, 2022 at 12:12:32AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> But if we *are* doing that then surely we should provide the full set of\n> functions. I.e. ALLOC() and ALLOC_ARRAY(), CALLOC() and CALLOC_ARRAY(),\n> and REALLOC() and REALLOC_ARRAY()?\n\nFWIW, I would be happy to see all of those (minus REALLOC(), as there is\nnot really any point in growing or shrinking something with a fixed\nsize).\n\nThe biggest argument against them that I can see is that:\n\n struct foo *x = malloc(sizeof(*x));\n\nis idiomatic C that newcomers to the project will easily understand,\nand:\n\n struct foo *x;\n ALLOC(x);\n\nis not. But it feels like we already crossed that bridge with\nALLOC_ARRAY(), etc.\n\n-Peff\n"},{"id":"468580","messageId":"221206.86v8mpcmnt.gmgdl@evledraar.gmail.com","threadId":"58897","inReplyTo":"Y46fHhD3xionHgPE@coredump.intra.peff.net","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-06T01:58:53Z","receivedAt":"2022-12-06T02:05:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 05 2022, Jeff King wrote:\n\n> On Tue, Dec 06, 2022 at 12:12:32AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> But if we *are* doing that then surely we should provide the full set of\n>> functions. I.e. ALLOC() and ALLOC_ARRAY(), CALLOC() and CALLOC_ARRAY(),\n>> and REALLOC() and REALLOC_ARRAY()?\n>\n> FWIW, I would be happy to see all of those (minus REALLOC(), as there is\n> not really any point in growing or shrinking something with a fixed\n> size).\n>\n> The biggest argument against them that I can see is that:\n>\n> struct foo *x = malloc(sizeof(*x));\n>\n> is idiomatic C that newcomers to the project will easily understand,\n> and:\n>\n> struct foo *x;\n> ALLOC(x);\n>\n> is not. But it feels like we already crossed that bridge with\n> ALLOC_ARRAY(), etc.\n\nThis is probably too ugly to exist, but if we are going to have more\nvariants maybe one that would allow use within declarations would be\nbetter, e.g.:\n\n\t\n\tdiff --git a/attr.c b/attr.c\n\tindex 42ad6de8c7c..43ade426e57 100644\n\t--- a/attr.c\n\t+++ b/attr.c\n\t@@ -666,11 +666,10 @@ static void handle_attr_line(struct attr_stack *res,\n\t \n\t static struct attr_stack *read_attr_from_array(const char **list)\n\t {\n\t-\tstruct attr_stack *res;\n\t+\tstruct attr_stack *INIT_CALLOC_ARRAY(res, 1);\n\t \tconst char *line;\n\t \tint lineno = 0;\n\t \n\t-\tCALLOC_ARRAY(res, 1);\n\t \twhile ((line = *(list++)) != NULL)\n\t \t\thandle_attr_line(res, line, \"[builtin]\", ++lineno,\n\t \t\t\t\t READ_ATTR_MACRO_OK);\n\tdiff --git a/git-compat-util.h b/git-compat-util.h\n\tindex a76d0526f79..932d0907f3f 100644\n\t--- a/git-compat-util.h\n\t+++ b/git-compat-util.h\n\t@@ -1089,6 +1089,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);\n\t \n\t #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))\n\t #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))\n\t+#define INIT_CALLOC_ARRAY(x, alloc) x = xcalloc((alloc), sizeof(*(x)))\n\t #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))\n\t \n\t #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \\\n\nI suspect (but haven't checked) that it might get us into the same\nparsing trouble as your UNUSED(name) macro.\n\nOr maybe:\n\t\n\tdiff --git a/attr.c b/attr.c\n\tindex 42ad6de8c7c..c3cb5c98bbf 100644\n\t--- a/attr.c\n\t+++ b/attr.c\n\t@@ -669 +669 @@ static struct attr_stack *read_attr_from_array(const char **list)\n\t-\tstruct attr_stack *res;\n\t+\tINIT_CALLOC_ARRAY(struct attr_stack *, res, 1);\n\t@@ -673 +672,0 @@ static struct attr_stack *read_attr_from_array(const char **list)\n\t-\tCALLOC_ARRAY(res, 1);\n\tdiff --git a/git-compat-util.h b/git-compat-util.h\n\tindex a76d0526f79..fd9af571dc9 100644\n\t--- a/git-compat-util.h\n\t+++ b/git-compat-util.h\n\t@@ -1091,0 +1092 @@ int xstrncmpz(const char *s, const char *t, size_t len);\n\t+#define INIT_CALLOC_ARRAY(d, x, alloc) d x = xcalloc((alloc), sizeof(*(x)))\n\t\nI think it's also worth considering just having the \"normal C' versions,\nbut we could transform anything that doesn't look lik ea narrow\nwhitelist of patterns into an error with coccinelle.\n\nI.e. if we're paranoid about \"v\" v.s. \"*var\" in the \"sizeof\" we could\nalso check & tranform that with coccinelle...\n"},{"id":"468598","messageId":"221206.86r0xdcm2u.gmgdl@evledraar.gmail.com","threadId":"58897","inReplyTo":"Y46M4oksPQkqwmTC@nand.local","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-06T02:12:08Z","receivedAt":"2022-12-06T02:17:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 05 2022, Taylor Blau wrote:\n\n> On Tue, Dec 06, 2022 at 08:57:21AM +0900, Junio C Hamano wrote:\n>> Taylor Blau writes:\n>>\n>> > In git.git, it is sometimes common to write something like:\n>> >\n>> > T *ptr;\n>> > CALLOC_ARRAY(ptr, 1);\n>> >\n>> > ...but that is confusing, since we're not initializing an array.\n>>\n>> Given that \"man calloc\" tells us that calloc takes two parameters,\n>>\n>> void *calloc(size_t nmemb, size_t size);\n>>\n>> I personally find CALLOC() that takes only a single parameter and is\n>> capable only to allocate a single element array very much confusing.\n>\n> Hmm. I have always considered \"calloc\" a mental shorthand for \"zero\n> initialize some bytes on the heap\". It seemed like you were in favor of\n> such a change in:\n>\n> https://lore.kernel.org/git/xmqq8rl8ivlb.fsf@gitster.g/\n>\n> ...but it's entirely possible that I misread your message, in which case\n> I would not be sad if you dropped this patch on the floor since I don't\n> feel that strongly about it.\n>\n> I'd be fine to call it CALLOC_ONE() or something, but I'm not sure at\n> that point if it's significantly better to write \"CALLOC_ONE(x)\" versus\n> \"CALLOC_ARRAY(foo, 1)\"\n>\n>> It _might_ be arguable that the order of the parameters CALLOC_ARRAY\n>> takes should have been reversed in that the number of elements in\n>> the array should come first just like in calloc(), while the pointer\n>> that is used to infer the size of an array element should come next,\n>> but that is water under the bridge.\n>\n> Yes, I agree that that would be better. But it would be frustrating to\n> make a tree-wide change of that magnitude at this point. So I agree it's\n> water under the bridge ;-).\n\nI'm not saying you *should*, but now that we use C99 macros we *could*\nalso make the \"1\" an optional argument. I.e. these would be the same\nthing:\n\n\tCALLOC(x)\n\tCALLOC(x, 1)\n\nAnd you could also do:\n\n\tCALLOC(x, 123)\n\nWhether that makes the interface even nastier is another matter.\n\nThat can be done by dispatching to an underlying function, and defining\nthe macro as:\n\n\t#define CALLOC(...) mycallocfn_1(__VA_ARGS__, NULL)\n\nI.e. for the above you'd get either \"x, NULL, NULL\", \"x, 1, NULL\", or\n\"x, 123, NULL\". The function could then manually check the arity.\n\nMaybe I've just been corrupted by reading the P99 library :)\n"},{"id":"468674","messageId":"Y4/6eDxUeSLYss/a@nand.local","threadId":"58897","inReplyTo":"Y46eVnYrcOGAbUhi@coredump.intra.peff.net","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-12-07T02:29:12Z","receivedAt":"2022-12-07T02:29:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Dec 05, 2022 at 08:43:50PM -0500, Jeff King wrote:\n> On Mon, Dec 05, 2022 at 05:36:25PM -0500, Taylor Blau wrote:\n>\n> > On Mon, Dec 05, 2022 at 10:01:11PM +0100, René Scharfe wrote:\n> > > This rule would turn this code:\n> > >\n> > > \tstruct foo *bar = xcalloc(1, sizeof(*bar));\n> > > \tint i;\n> > >\n> > > ... into:\n> > >\n> > > \tstruct foo *bar;\n> > > \tCALLOC(bar);\n> > > \tint i;\n> > >\n> > > ... which violates the coding guideline to not mix declarations and\n> > > statements (-Wdeclaration-after-statement).\n> >\n> > Yeah, I was wondering about this myself when I wrote this part of the\n> > Coccinelle patch.\n> >\n> > Is there an intelligent way to tell it to put the first statement after\n> > all declarations? I couldn't find anything after a quick scan of the\n> > documentation nor our own patches.\n>\n> It feels like generating the code as above is not the end of the world.\n> The most valuable thing that coccinelle is doing here is _finding_ the\n> location, and telling you \"it's supposed to be like this\". It is great\n> when the \"this\" post-image is perfect and doesn't need further tweaking.\n\nI have to agree. If Coccinelle can generate the right output; great. But\nif it can't, the amount of additional work to reorganize an already\ngenerated and mostly correct *.patch from the tool seems minimal by\ncomparison.\n\n> But if the compiler then reminds you \"hey, you need to go a bit further\n> manually\", that doesn't seem so bad. In other words, I would be happy to\n> follow that work flow if I introduced a bare xcalloc(). My only worry is\n> that somebody less experienced with the project (or with C) would get\n> confused.\n\nAgreed.\n\nThanks,\nTaylor\n"},{"id":"468675","messageId":"Y4/7txDTWK5nGPDu@nand.local","threadId":"58897","inReplyTo":"221206.868rjle7za.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-12-07T02:34:31Z","receivedAt":"2022-12-07T02:34:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Dec 06, 2022 at 12:12:32AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> First, when you add *.pending.cocci rules they shouldn't be pseudocode,\n> but things that are too large to apply right now. I think my recent\n> 041df69edd3 (Merge branch 'ab/fewer-the-index-macros', 2022-11-28) is a\n> good example.\n\nAgreed, but I do tend to consider this patch too-large to apply right\nnow.\n\nWhether or not 2k lines of diff is review-able or not (and FWIW, I think\nthat your recent Coccinelle-generated patches were quite easy to review\npretty quickly), I think there's a bigger question of whether or not we\nwant to make such a sweeping, tree-wide change. And assuming the answer\nto that is \"yes\", there's is also a question of timing. Proposing it\ntowards the middle or end of a release cycle seems in bad taste.\n\nBut I think splitting this discussion up into, \"should we introduce\nsomething like CALLOC() into our style conventions?\" and \"do we want to\napply this everywhere?\" is worth doing. The former doesn't seem to take\na ton of time away from polishing the release candidates, and the latter\ncan be done only after the former has settled.\n\nSo the decision to make this a *.pending.cocci rule was definitely\nintentional in this case.\n\n> Second, we have test support for rules now, see\n> contrib/coccinelle/tests/. You just need to create a\n> contrib/coccinelle/tests/xcalloc.pending.c, and have the expected\n> post-image in contrib/coccinelle/tests/xcalloc.pending.res. Please add\n> one of those. We don't have them for existing rules, but it really helps\n> to assert & review new rules.\n>\n> The various edge cases that your current *.cocci doesn't compile on\n> etc. should go into that test file.\n\nThanks for mentioning.\n\n> Third, the resulting patch is currently ~2k lines. Can we really not\n> make it non-pending with some whitelisting/gblacklisting. E.g. see this\n> out-of-tree patch for an example of opting out certain functions:\n> https://github.com/avar/git/commit/bedd0323e9b\n\nSee above.\n\n> Fourth, I must say I'm kind of negative on the proposed change. I.e. the\n> foot-gun is definitely worth solving, but why not just create a\n> coccinelle rule to narrowly address that? In that case we can presumably\n> start with it non-pending, as we don't have that many of them.\n>\n> On the notion that we need to special-case:\n>\n> \t- CALLOC_ARRAY(ptr, 1)\n> \t+ CALLOC(ptr)\n>\n> Because an array of one is \"not an array\" I don't really buy it. The\n> calloc() interface itself exposes that, so I don't see why we'd consider\n> those separate on the API level for these wrapper macros.\n\nI think the point is that it's just weird. Yes, an array of just a\nsingle element on the heap is indistinguishable from asking malloc() to\ngive me the same bytes and then memset() them myself, just as it's\nindistinguishable from calloc()'ing the right number of bytes for a\nsingle structure to begin with.\n\nBut whether or not the two are indistinguishable doesn't mean that it\nmakes intuitive sense, and that is the goal of CALLOC().\n\nThanks,\nTaylor\n"},{"id":"468676","messageId":"Y4/8FzWiYasWemSW@nand.local","threadId":"58897","inReplyTo":"Y46fHhD3xionHgPE@coredump.intra.peff.net","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-12-07T02:36:07Z","receivedAt":"2022-12-07T02:36:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Dec 05, 2022 at 08:47:10PM -0500, Jeff King wrote:\n> On Tue, Dec 06, 2022 at 12:12:32AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n> > But if we *are* doing that then surely we should provide the full set of\n> > functions. I.e. ALLOC() and ALLOC_ARRAY(), CALLOC() and CALLOC_ARRAY(),\n> > and REALLOC() and REALLOC_ARRAY()?\n>\n> FWIW, I would be happy to see all of those (minus REALLOC(), as there is\n> not really any point in growing or shrinking something with a fixed\n> size).\n\nSame.\n\n> The biggest argument against them that I can see is that:\n>\n> struct foo *x = malloc(sizeof(*x));\n>\n> is idiomatic C that newcomers to the project will easily understand,\n> and:\n>\n> struct foo *x;\n> ALLOC(x);\n>\n> is not. But it feels like we already crossed that bridge with\n> ALLOC_ARRAY(), etc.\n\nYeah, I agree that it might be off-putting to have that convention\npervade throughout the tree. On the other hand, though, we can clearly\ndocument that in CodingGuidelines, and make Coccinelle rules that\nindicate our preferences.\n\nAnd indeed, ALLOC_ARRAY() and friends is a useful test-case for gauging\nhow something like this might go. I don't think we necessarily need to\ndo all of that in the same set of patches (though I'm not opposed to\ndoing so, either), but I wouldn't mind getting there in the end, one way\nor another.\n\nThanks,\nTaylor\n"},{"id":"468677","messageId":"Y4/8ilFBfGApR8YR@nand.local","threadId":"58897","inReplyTo":"xmqqv8mpnwjp.fsf@gitster.g","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-12-07T02:38:02Z","receivedAt":"2022-12-07T02:38:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Dec 06, 2022 at 10:35:54AM +0900, Junio C Hamano wrote:\n> Jeff King writes:\n>\n> > So you can think of \"allocate this one element and zero it\" as \"calloc,\n> > but don't multiply\" or as \"malloc, but zero\". Naming it CALLOC() is\n> > thinking of it as the former. If we think of it as the latter it could\n> > perhaps be MALLOCZ() or something. I don't know if that name is too\n> > subtle or not. We have xmemdupz(), which is basically the same thing;\n> > it's only a zero-terminator, but that is because we are writing non-zero\n> > bytes in the rest of it. Mostly I'd worry that it is easy to glance past\n> > the \"Z\".\n>\n> I think the name for the former would be CALLOC_ONE(), as I would\n> rephrase it as \"calloc, but just one element\". I agree MALLOCZ()\n> would be fine for the other interpretation, and I do not have much\n> problem as much problem with the name as calling it CALLOC().\n\nBetween CALLOC_ONE() and MALLOCZ(), I prefer the latter for brevity. But\nbetween that and CALLOC(), I prefer the latter, since \"CALLOC()\" reminds\nme of the zero-initialization of calloc()-proper, and the \"Z\" in\n\"MALLOCZ()\" feels easy-ish to miss.\n\nThanks,\nTaylor\n"},{"id":"468679","messageId":"221207.86bkofc1qw.gmgdl@evledraar.gmail.com","threadId":"58897","inReplyTo":"Y4/7txDTWK5nGPDu@nand.local","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T03:17:07Z","receivedAt":"2022-12-07T03:49:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 06 2022, Taylor Blau wrote:\n\n> On Tue, Dec 06, 2022 at 12:12:32AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> First, when you add *.pending.cocci rules they shouldn't be pseudocode,\n>> but things that are too large to apply right now. I think my recent\n>> 041df69edd3 (Merge branch 'ab/fewer-the-index-macros', 2022-11-28) is a\n>> good example.\n>\n> Agreed, but I do tend to consider this patch too-large to apply right\n> now.\n\nI think it's OK if a *pending* patch is 10k lines or whatever, as long\nas the result of applying it compiles. I.e. if it doesn't compile the\nsemantic patch is broken, and doesn't really help to clarify what the\nproposed change is.\n\nWhether the change itself is too large to be worth it or whatever is\nanother matter, but we can always consider it advisory.\n\nE.g. consider a change to remove braces from if/else per our coding\nstyle, we don't have such a *.cocci now, but if we did it would be a\nvery large patch. It might still be useful in *.pending.cocci, e.g. to\nrun new code through it.\n\n> Whether or not 2k lines of diff is review-able or not (and FWIW, I think\n> that your recent Coccinelle-generated patches were quite easy to review\n> pretty quickly), I think there's a bigger question of whether or not we\n> want to make such a sweeping, tree-wide change. And assuming the answer\n> to that is \"yes\", there's is also a question of timing. Proposing it\n> towards the middle or end of a release cycle seems in bad taste.\n>\n> But I think splitting this discussion up into, \"should we introduce\n> something like CALLOC() into our style conventions?\" and \"do we want to\n> apply this everywhere?\" is worth doing. The former doesn't seem to take\n> a ton of time away from polishing the release candidates, and the latter\n> can be done only after the former has settled.\n>\n> So the decision to make this a *.pending.cocci rule was definitely\n> intentional in this case.\n\nYes. A sweeping change like this should definitely start there (although\nsee below).\n\n> [...]\n>> Third, the resulting patch is currently ~2k lines. Can we really not\n>> make it non-pending with some whitelisting/gblacklisting. E.g. see this\n>> out-of-tree patch for an example of opting out certain functions:\n>> https://github.com/avar/git/commit/bedd0323e9b\n>\n> See above.\n\nWhat I'm demonstrating with that commit is that you can make a\nnon-pending patch by blacklisting the functions, filenames etc. that are\nknown to be outstanding.\n\nThe advantage of doing that being that you'd apply it for any new code\noutside those scopes.\n\nSo re the \"see above\" it's an explicit end-run around the \"do we want to\napply this everywhere?\".\n\n>> Fourth, I must say I'm kind of negative on the proposed change. I.e. the\n>> foot-gun is definitely worth solving, but why not just create a\n>> coccinelle rule to narrowly address that? In that case we can presumably\n>> start with it non-pending, as we don't have that many of them.\n>>\n>> On the notion that we need to special-case:\n>>\n>> \t- CALLOC_ARRAY(ptr, 1)\n>> \t+ CALLOC(ptr)\n>>\n>> Because an array of one is \"not an array\" I don't really buy it. The\n>> calloc() interface itself exposes that, so I don't see why we'd consider\n>> those separate on the API level for these wrapper macros.\n>\n> I think the point is that it's just weird. Yes, an array of just a\n> single element on the heap is indistinguishable from asking malloc() to\n> give me the same bytes and then memset() them myself, just as it's\n> indistinguishable from calloc()'ing the right number of bytes for a\n> single structure to begin with.\n>\n> But whether or not the two are indistinguishable doesn't mean that it\n> makes intuitive sense, and that is the goal of CALLOC().\n\nI understand that's the goal, I just don't get how it's intuitive.\n\nFirst we had REALLOC_ARRAY(), which is the same as a reallocarray(3)\nwhere the \"size\" is computed for you based on the \"ptr\", which we know\nto be a variable name in our case. Later we had ALLOC_ARRAY() and\nCALLOC_ARRAY(), which aim to provide the same sort of interface for\nmalloc() and calloc().\n\nThe reallocarray() function comes from OpenBSD, where it's always\nsupported nmemb = 1:\nhttps://man.openbsd.org/OpenBSD-5.9/malloc.3#reallocarray~2\n\nIt has always been redundant to use it for nmemb=1, but as arrays in C\ndon't have containers (unlike some higher-level lanugages) it would be\nodd to disallow it, and force the user to do:\n\n\tmem = n == 1 ? realloc(mem, sz) : reallocarray(mem, n, sz);\n\nInstead of:\n\n\tmem = reallocarray(mem, n, sz);\n\nI.e. you don't need the overlfow check for n * sz if n == 1, but\ntreating it as a special-case is a hassle.\n"},{"id":"468680","messageId":"221207.867cz3c1cp.gmgdl@evledraar.gmail.com","threadId":"58897","inReplyTo":"Y4/6eDxUeSLYss/a@nand.local","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T03:51:04Z","receivedAt":"2022-12-07T03:57:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 06 2022, Taylor Blau wrote:\n\n> On Mon, Dec 05, 2022 at 08:43:50PM -0500, Jeff King wrote:\n>> On Mon, Dec 05, 2022 at 05:36:25PM -0500, Taylor Blau wrote:\n>>\n>> > On Mon, Dec 05, 2022 at 10:01:11PM +0100, René Scharfe wrote:\n>> > > This rule would turn this code:\n>> > >\n>> > > \tstruct foo *bar = xcalloc(1, sizeof(*bar));\n>> > > \tint i;\n>> > >\n>> > > ... into:\n>> > >\n>> > > \tstruct foo *bar;\n>> > > \tCALLOC(bar);\n>> > > \tint i;\n>> > >\n>> > > ... which violates the coding guideline to not mix declarations and\n>> > > statements (-Wdeclaration-after-statement).\n>> >\n>> > Yeah, I was wondering about this myself when I wrote this part of the\n>> > Coccinelle patch.\n>> >\n>> > Is there an intelligent way to tell it to put the first statement after\n>> > all declarations? I couldn't find anything after a quick scan of the\n>> > documentation nor our own patches.\n>>\n>> It feels like generating the code as above is not the end of the world.\n>> The most valuable thing that coccinelle is doing here is _finding_ the\n>> location, and telling you \"it's supposed to be like this\". It is great\n>> when the \"this\" post-image is perfect and doesn't need further tweaking.\n>\n> I have to agree. If Coccinelle can generate the right output; great. But\n> if it can't, the amount of additional work to reorganize an already\n> generated and mostly correct *.patch from the tool seems minimal by\n> comparison.\n\nIt can, but you need to write your semantic patch to match your\nintent. If you write e.g.:\n\n\t- int x;\n + int y;\n + foo();\n\nThat means \"add the int y and foo() line right after that \"int x\" line\nyou removed.\n\nWhereas what you want in this case is closer to:\n\n - match the \"int x\" line\n - remove or amend it\n - skip past all subsequent declarations\n - skip past all code that isn't referring to the \"x variable?\n - insert the \"CALLOC_ARRAY\" (or whatever) before that first \"x\" use.\n\nI don't know offhand how to match this, but presumably it's some mixture\nof the wildcard syntax (\"...\", \"<... ...>\" etc.) and matching a\n\"statement\", or maybe marking the \"int x\" with the \"@pos\" syntax, and\nreferring to that position again.\n\nI usually just browse through the coccinelle.git for *.cocci examples\nand/or read the PDF (*not* the manual page, which discusses almost none\nof the syntax) documentation.\n\nSee:\n\n\tgit grep -F '...' -- contrib/coccinelle \n\nFor some in-tree use of this, the unused.cocci I added recently is\nprobably the closest equivalent to what you'll want.\n\n\n"},{"id":"468689","messageId":"Y5AsWvLSwy7EgiHT@coredump.intra.peff.net","threadId":"58897","inReplyTo":"221206.86v8mpcmnt.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-07T06:02:02Z","receivedAt":"2022-12-07T06:02:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 06, 2022 at 02:58:53AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> This is probably too ugly to exist, but if we are going to have more\n> variants maybe one that would allow use within declarations would be\n> better, e.g.:\n> \n> \t\n> \tdiff --git a/attr.c b/attr.c\n> \tindex 42ad6de8c7c..43ade426e57 100644\n> \t--- a/attr.c\n> \t+++ b/attr.c\n> \t@@ -666,11 +666,10 @@ static void handle_attr_line(struct attr_stack *res,\n> \t \n> \t static struct attr_stack *read_attr_from_array(const char **list)\n> \t {\n> \t-\tstruct attr_stack *res;\n> \t+\tstruct attr_stack *INIT_CALLOC_ARRAY(res, 1);\n\nYeah. While clever, I would agree that is probably too ugly to exist.\n\n-Peff\n"},{"id":"468690","messageId":"Y5AtdwGdWrDQxuuJ@coredump.intra.peff.net","threadId":"58897","inReplyTo":"221206.86r0xdcm2u.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-07T06:06:47Z","receivedAt":"2022-12-07T06:06:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 06, 2022 at 03:12:08AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> I'm not saying you *should*, but now that we use C99 macros we *could*\n> also make the \"1\" an optional argument. I.e. these would be the same\n> thing:\n> \n> \tCALLOC(x)\n> \tCALLOC(x, 1)\n> \n> And you could also do:\n> \n> \tCALLOC(x, 123)\n> \n> Whether that makes the interface even nastier is another matter.\n\nLike the INIT proposal you mentioned, I tend to think this is clever but\nnot really a good idea in the long run. I don't mind magic if it's\ngiving us a big benefit, but I don't think that's the case here. :)\n\n-Peff\n"},{"id":"468691","messageId":"Y5Aty8p4617CZu1C@coredump.intra.peff.net","threadId":"58897","inReplyTo":"Y4/8ilFBfGApR8YR@nand.local","subject":"Re: [PATCH] git-compat-util.h: introduce CALLOC(x)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-07T06:08:11Z","receivedAt":"2022-12-07T06:08:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 06, 2022 at 09:38:02PM -0500, Taylor Blau wrote:\n\n> On Tue, Dec 06, 2022 at 10:35:54AM +0900, Junio C Hamano wrote:\n> > Jeff King writes:\n> >\n> > > So you can think of \"allocate this one element and zero it\" as \"calloc,\n> > > but don't multiply\" or as \"malloc, but zero\". Naming it CALLOC() is\n> > > thinking of it as the former. If we think of it as the latter it could\n> > > perhaps be MALLOCZ() or something. I don't know if that name is too\n> > > subtle or not. We have xmemdupz(), which is basically the same thing;\n> > > it's only a zero-terminator, but that is because we are writing non-zero\n> > > bytes in the rest of it. Mostly I'd worry that it is easy to glance past\n> > > the \"Z\".\n> >\n> > I think the name for the former would be CALLOC_ONE(), as I would\n> > rephrase it as \"calloc, but just one element\". I agree MALLOCZ()\n> > would be fine for the other interpretation, and I do not have much\n> > problem as much problem with the name as calling it CALLOC().\n> \n> Between CALLOC_ONE() and MALLOCZ(), I prefer the latter for brevity. But\n> between that and CALLOC(), I prefer the latter, since \"CALLOC()\" reminds\n> me of the zero-initialization of calloc()-proper, and the \"Z\" in\n> \"MALLOCZ()\" feels easy-ish to miss.\n\nFWIW, I agree with your preferences here. But if people think CALLOC()\nis misleading or badly named, I'm OK with MALLOCZ(). I guess maybe it is\nALLOCZ() to go with ALLOC_ARRAY(), and a potential ALLOC() if we choose\nto have one.\n\n-Peff\n"}]}