{"thread":{"id":"36954","subject":"[PATCH] alloc.c: remove alloc_raw_commit_node() function","startedAt":"2014-06-18T19:52:46Z","lastAt":"2014-06-19T20:32:04Z","messageCount":6,"participants":["Ramsay Jones","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"244575","messageId":"53A1EE0E.6040000@ramsay1.demon.co.uk","threadId":"36954","inReplyTo":null,"subject":"[PATCH] alloc.c: remove alloc_raw_commit_node() function","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-06-18T19:52:46Z","receivedAt":"2014-06-18T19:52:46Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"In order to encapsulate the setting of the unique commit index, commit\n969eba63 (\"commit: push commit_index update into alloc_commit_node\",\n10-06-2014) introduced a (logically private) intermediary allocator\nfunction. However, this function (alloc_raw_commit_node()) was declared\nas a public function, which undermines its entire purpose.\n\nRemove the alloc_raw_commit_node() function and inline its code into\nthe (public) alloc_commit_node() function.\n\nNoticed by sparse (\"symbol 'alloc_raw_commit_node' was not declared.\nShould it be static?\").\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nHi Jeff,\n\nI noticed this while it was still in 'pu', but got distracted and\ndidn't send this in time ... sorry about that! :(\n\nMy first attempt at fixing this involved changing the DEFINE_ALLOCATOR\nmacro to include a 'scope' parameter so that I could declare the\nraw_commit allocator 'static'. Unfortunately, I could not pass the\nextern keyword as the scope parameter to all the other allocators,\nbecause that made sparse even more upset - you can't use extern on\na function _definition_. That meant passing an empty argument (or a\ncomment token) to the scope parameter. This worked for gcc 4.8.2 and\nclang 3.4, but I was a little concerned about portability.\n\nThis seems a better solution to me. Having said that ... as I'm typing\nthis I realized that I could have removed the 'commit_count' variable\nand used 'commit_allocs' to set c->index instead! :-P Oh well ...\n\nATB,\nRamsay Jones\n\n alloc.c | 22 ++++++++++++++++++----\n 1 file changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex eb22a45..124d710 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -47,16 +47,30 @@ union any_object {\n \n DEFINE_ALLOCATOR(blob, struct blob)\n DEFINE_ALLOCATOR(tree, struct tree)\n-DEFINE_ALLOCATOR(raw_commit, struct commit)\n DEFINE_ALLOCATOR(tag, struct tag)\n DEFINE_ALLOCATOR(object, union any_object)\n \n+static unsigned int commit_allocs;\n+\n void *alloc_commit_node(void)\n {\n \tstatic int commit_count;\n-\tstruct commit *c = alloc_raw_commit_node();\n+\tstatic int nr;\n+\tstatic struct commit *block;\n+\tstruct commit *c;\n+\tvoid *ret;\n+\n+\tif (!nr) {\n+\t\tnr = BLOCKING;\n+\t\tblock = xmalloc(BLOCKING * sizeof(struct commit));\n+\t}\n+\tnr--;\n+\tcommit_allocs++;\n+\tret = block++;\n+\tmemset(ret, 0, sizeof(struct commit));\n+\tc = (struct commit *) ret;\n \tc->index = commit_count++;\n-\treturn c;\n+\treturn ret;\n }\n \n static void report(const char *name, unsigned int count, size_t size)\n@@ -72,7 +86,7 @@ void alloc_report(void)\n {\n \tREPORT(blob, struct blob);\n \tREPORT(tree, struct tree);\n-\tREPORT(raw_commit, struct commit);\n+\tREPORT(commit, struct commit);\n \tREPORT(tag, struct tag);\n \tREPORT(object, union any_object);\n }\n-- \n2.0.0\n"},{"id":"244582","messageId":"20140618200854.GA23098@sigill.intra.peff.net","threadId":"36954","inReplyTo":"53A1EE0E.6040000@ramsay1.demon.co.uk","subject":"Re: [PATCH] alloc.c: remove alloc_raw_commit_node() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:08:55Z","receivedAt":"2014-06-18T20:08:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 18, 2014 at 08:52:46PM +0100, Ramsay Jones wrote:\n\n> In order to encapsulate the setting of the unique commit index, commit\n> 969eba63 (\"commit: push commit_index update into alloc_commit_node\",\n> 10-06-2014) introduced a (logically private) intermediary allocator\n> function. However, this function (alloc_raw_commit_node()) was declared\n> as a public function, which undermines its entire purpose.\n> \n> Remove the alloc_raw_commit_node() function and inline its code into\n> the (public) alloc_commit_node() function.\n> \n> Noticed by sparse (\"symbol 'alloc_raw_commit_node' was not declared.\n> Should it be static?\").\n> \n> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n> ---\n> \n> Hi Jeff,\n> \n> I noticed this while it was still in 'pu', but got distracted and\n> didn't send this in time ... sorry about that! :(\n\nYeah, I noticed it while writing the patch but decided it wasn't worth\nthe trouble to deal with (since after all, it's not advertised to any\ncallers, the very thing that sparse is complaining about. :) ).\n\nI don't mind fixing it, though I really don't like repeating the\ncontents of DEFINE_ALLOCATOR. I know it hasn't changed in a while, but\nit just feels wrong.\n\n> My first attempt at fixing this involved changing the DEFINE_ALLOCATOR\n> macro to include a 'scope' parameter so that I could declare the\n> raw_commit allocator 'static'. Unfortunately, I could not pass the\n> extern keyword as the scope parameter to all the other allocators,\n> because that made sparse even more upset - you can't use extern on\n> a function _definition_. That meant passing an empty argument (or a\n> comment token) to the scope parameter. This worked for gcc 4.8.2 and\n> clang 3.4, but I was a little concerned about portability.\n\nYeah, passing an empty argument was my first thought, but I don't know\noffhand if there are portability concerns.\n\nYou could also have DEFINE_ALLOCATOR just fill in the body, and do:\n\n  struct blob *alloc_blob_node(void)\n  {\n\tDEFINE_ALLOCATOR(struct blob);\n  }\n\nor similar.\n\n-Peff\n"},{"id":"244606","messageId":"53A2131A.30900@ramsay1.demon.co.uk","threadId":"36954","inReplyTo":"20140618200854.GA23098@sigill.intra.peff.net","subject":"Re: [PATCH] alloc.c: remove alloc_raw_commit_node() function","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-06-18T22:30:50Z","receivedAt":"2014-06-18T22:30:50Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 18/06/14 21:08, Jeff King wrote:\n> On Wed, Jun 18, 2014 at 08:52:46PM +0100, Ramsay Jones wrote:\n> \n[snip]\n> Yeah, I noticed it while writing the patch but decided it wasn't worth\n> the trouble to deal with (since after all, it's not advertised to any\n> callers, the very thing that sparse is complaining about. :) ).\n> \n> I don't mind fixing it, though I really don't like repeating the\n> contents of DEFINE_ALLOCATOR. I know it hasn't changed in a while, but\n> it just feels wrong.\n\nSo, the patch below is a slight variation on the original patch.\nI'm still slightly concerned about portability, but given that it\nhas been at least a decade since I last used a (pre-ANSI) compiler\nwhich had a problem with this ...\n\n[I have several versions of the C standard that I can use to check\nup on the legalise, but I'm not sure I can be bothered! ;-) ]\n\nATB,\nRamsay Jones\n\n-- >8 --\nSubject: [PATCH] alloc.c: make alloc_raw_commit_node() a static function\n\nIn order to encapsulate the setting of the unique commit index, commit\n969eba63 (\"commit: push commit_index update into alloc_commit_node\",\n10-06-2014) introduced a (logically private) intermediary allocator\nfunction. However, this function (alloc_raw_commit_node()) was declared\nas a public function, which undermines its entire purpose.\n\nAdd a scope parameter to the DEFINE_ALLOCATOR macro to allow the\nraw commit allocator definition to include the 'static' qualifier.\n\nNoticed by sparse (\"symbol 'alloc_raw_commit_node' was not declared.\nShould it be static?\").\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n alloc.c | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex eb22a45..5392d13 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -18,9 +18,12 @@\n \n #define BLOCKING 1024\n \n-#define DEFINE_ALLOCATOR(name, type)\t\t\t\t\\\n+#define PUBLIC\n+#define PRIVATE static\n+\n+#define DEFINE_ALLOCATOR(scope, name, type)\t\t\t\\\n static unsigned int name##_allocs;\t\t\t\t\\\n-void *alloc_##name##_node(void)\t\t\t\t\t\\\n+scope void *alloc_##name##_node(void)\t\t\t\t\\\n {\t\t\t\t\t\t\t\t\\\n \tstatic int nr;\t\t\t\t\t\t\\\n \tstatic type *block;\t\t\t\t\t\\\n@@ -45,11 +48,11 @@ union any_object {\n \tstruct tag tag;\n };\n \n-DEFINE_ALLOCATOR(blob, struct blob)\n-DEFINE_ALLOCATOR(tree, struct tree)\n-DEFINE_ALLOCATOR(raw_commit, struct commit)\n-DEFINE_ALLOCATOR(tag, struct tag)\n-DEFINE_ALLOCATOR(object, union any_object)\n+DEFINE_ALLOCATOR(PUBLIC, blob, struct blob)\n+DEFINE_ALLOCATOR(PUBLIC, tree, struct tree)\n+DEFINE_ALLOCATOR(PRIVATE, raw_commit, struct commit)\n+DEFINE_ALLOCATOR(PUBLIC, tag, struct tag)\n+DEFINE_ALLOCATOR(PUBLIC, object, union any_object)\n \n void *alloc_commit_node(void)\n {\n-- \n2.0.0\n"},{"id":"244634","messageId":"20140619091924.GA29478@sigill.intra.peff.net","threadId":"36954","inReplyTo":"53A2131A.30900@ramsay1.demon.co.uk","subject":"Re: [PATCH] alloc.c: remove alloc_raw_commit_node() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T09:19:24Z","receivedAt":"2014-06-19T09:19:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 18, 2014 at 11:30:50PM +0100, Ramsay Jones wrote:\n\n> So, the patch below is a slight variation on the original patch.\n> I'm still slightly concerned about portability, but given that it\n> has been at least a decade since I last used a (pre-ANSI) compiler\n> which had a problem with this ...\n\nFor a while some people were compiling git with pretty antique\ncompilers, but I do not know if that is the case any more (Junio noted\nrecently that we have had trailing commas on arrays and enums in\nbuiltin/clean.c for the past year, and nobody has complained).\n\nMaybe those systems have all died off, or maybe those people only\nupgrade every few years, and we are due for an onslaught of portability\nfixes soon. :)\n\n> [I have several versions of the C standard that I can use to check\n> up on the legalise, but I'm not sure I can be bothered! ;-) ]\n\nIt was actually pretty easy to find. I only have C99, but \"empty macro\narguments\" are listed as one of the changes since C89 in the foreward.\n\n> diff --git a/alloc.c b/alloc.c\n> index eb22a45..5392d13 100644\n> --- a/alloc.c\n> +++ b/alloc.c\n> @@ -18,9 +18,12 @@\n>  \n>  #define BLOCKING 1024\n>  \n> -#define DEFINE_ALLOCATOR(name, type)\t\t\t\t\\\n> +#define PUBLIC\n> +#define PRIVATE static\n> +\n> +#define DEFINE_ALLOCATOR(scope, name, type)\t\t\t\\\n\nI am not sure offhand whether this is more portable or not (it would\ndepend on order of macro expansion, and I am not brave enough to read\nthrough the preprocessor section of the standard carefully). Quick\nreading online suggests that it's OK, but that an extra level of macro\nexpansion would not be. And of course the Internet is never wrong. :)\n\nI'm inclined to go with it, and deal with it later if we get a\nportability complaint (at which point we are no worse off than we are\nright now).\n\n-Peff\n"},{"id":"244635","messageId":"53A2B39A.6040902@ramsay1.demon.co.uk","threadId":"36954","inReplyTo":"20140619091924.GA29478@sigill.intra.peff.net","subject":"Re: [PATCH] alloc.c: remove alloc_raw_commit_node() function","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-06-19T09:55:38Z","receivedAt":"2014-06-19T09:55:38Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 19/06/14 10:19, Jeff King wrote:\n> On Wed, Jun 18, 2014 at 11:30:50PM +0100, Ramsay Jones wrote:\n> \n>> So, the patch below is a slight variation on the original patch.\n>> I'm still slightly concerned about portability, but given that it\n>> has been at least a decade since I last used a (pre-ANSI) compiler\n>> which had a problem with this ...\n> \n> For a while some people were compiling git with pretty antique\n> compilers, but I do not know if that is the case any more (Junio noted\n> recently that we have had trailing commas on arrays and enums in\n> builtin/clean.c for the past year, and nobody has complained).\n> \n> Maybe those systems have all died off, or maybe those people only\n> upgrade every few years, and we are due for an onslaught of portability\n> fixes soon. :)\n\n:-P You never know ...\n\n>> [I have several versions of the C standard that I can use to check\n>> up on the legalise, but I'm not sure I can be bothered! ;-) ]\n> \n> It was actually pretty easy to find. I only have C99, but \"empty macro\n> arguments\" are listed as one of the changes since C89 in the foreward.\n\nAh yes, having prompted me to look, it took all of 2 minutes to find it\nin the C11 .pdf file I have right here ...\n\n>> diff --git a/alloc.c b/alloc.c\n>> index eb22a45..5392d13 100644\n>> --- a/alloc.c\n>> +++ b/alloc.c\n>> @@ -18,9 +18,12 @@\n>>  \n>>  #define BLOCKING 1024\n>>  \n>> -#define DEFINE_ALLOCATOR(name, type)\t\t\t\t\\\n>> +#define PUBLIC\n>> +#define PRIVATE static\n>> +\n>> +#define DEFINE_ALLOCATOR(scope, name, type)\t\t\t\\\n> \n> I am not sure offhand whether this is more portable or not (it would\n> depend on order of macro expansion, and I am not brave enough to read\n> through the preprocessor section of the standard carefully).\n\nI suspect it is _slightly_ more portable, but I wouldn't bet any money\non it! I may take a look at the standard (it wouldn't be the first time\nI've looked this up), but it would not help in this situation anyway.\nThe fact that a given compiler does/does-not conform to the standard\nis useless knowledge if we need to support them.\n\n>                                                               Quick\n> reading online suggests that it's OK, but that an extra level of macro\n> expansion would not be. And of course the Internet is never wrong. :)\n\n;-)\n\n> I'm inclined to go with it, and deal with it later if we get a\n> portability complaint (at which point we are no worse off than we are\n> right now).\n\nHmm, well my initial reaction was to send the more conservative patch\nfirst. I wasn't so worried about inlining the code from the macro\n(I doubt it will change), but I understand (and would often agree with)\nyour concern.\n\nI would be happy with either solution, so I'll let yourself and Junio\ndecide! (sloping shoulders, or what. :) ).\n\nATB,\nRamsay Jones\n"},{"id":"244983","messageId":"xmqq1tuc8ypr.fsf@gitster.dls.corp.google.com","threadId":"36954","inReplyTo":"20140619091924.GA29478@sigill.intra.peff.net","subject":"Re: [PATCH] alloc.c: remove alloc_raw_commit_node() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-19T20:32:04Z","receivedAt":"2014-06-19T20:32:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> For a while some people were compiling git with pretty antique\n> compilers, but I do not know if that is the case any more (Junio noted\n> recently that we have had trailing commas on arrays and enums in\n> builtin/clean.c for the past year, and nobody has complained).\n\nIIRC, the problematic is a trailing comma in enum definitions, not\narray initialisations.\n"}]}