{"thread":{"id":"35403","subject":"[PATCH 0/2] commit-slab cleanups","startedAt":"2013-11-25T19:01:59Z","lastAt":"2013-12-02T15:35:13Z","messageCount":18,"participants":["Thomas Rast","Junio C Hamano","Jonathan Nieder","Eric Sunshine","Duy Nguyen","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"231082","messageId":"cover.1385405977.git.tr@thomasrast.ch","threadId":"35403","inReplyTo":null,"subject":"[PATCH 0/2] commit-slab cleanups","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-25T19:01:59Z","receivedAt":"2013-11-25T19:01:59Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"I gathered these while writing an \"all merge bases simultaneously\"\nalgorithm.  Turns out this fancy algorithm loses vs. simply calling\nget_merge_bases() a lot, so I dropped that.  But the cleanups seem\nvalid anyway.\n\n\nThomas Rast (2):\n  commit-slab: document clear_$slabname()\n  commit-slab: declare functions \"static inline\"\n\n commit-slab.h | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\n-- \n1.8.5.rc3.397.g2a3acd5\n"},{"id":"231084","messageId":"7f773c5c5ea16b19840f67ba99961be132940d32.1385405977.git.tr@thomasrast.ch","threadId":"35403","inReplyTo":"cover.1385405977.git.tr@thomasrast.ch","subject":"[PATCH 1/2] commit-slab: document clear_$slabname()","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-25T19:02:00Z","receivedAt":"2013-11-25T19:02:00Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The clear_$slabname() function was only documented by source code so\nfar.  Write something about it.\n\nSigned-off-by: Thomas Rast <tr@thomasrast.ch>\n---\n commit-slab.h | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex d4c8286..d77aaea 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -24,6 +24,10 @@\n  *   to each commit. 'stride' specifies how big each array is.  The slab\n  *   that id initialied by the variant without \"_with_stride\" associates\n  *   each commit with an array of one integer.\n+ *\n+ * - void clear_indegree(struct indegree *);\n+ *\n+ *   Free the slab's data structures.\n  */\n \n /* allocate ~512kB at once, allowing for malloc overhead */\n-- \n1.8.5.rc3.397.g2a3acd5\n"},{"id":"231083","messageId":"f4d1ff9f487f797da35faa86c72d11832903a50d.1385405977.git.tr@thomasrast.ch","threadId":"35403","inReplyTo":"cover.1385405977.git.tr@thomasrast.ch","subject":"[PATCH 2/2] commit-slab: declare functions \"static inline\"","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-25T19:02:01Z","receivedAt":"2013-11-25T19:02:01Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"This shuts up compiler warnings about unused functions.\n\nWhile there, also remove the redundant second declaration of\nstat_##slabname##realloc.\n\nSigned-off-by: Thomas Rast <tr@thomasrast.ch>\n---\n commit-slab.h | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex d77aaea..d5c353e 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -45,8 +45,8 @@ struct slabname {\t\t\t\t\t\t\t\\\n };\t\t\t\t\t\t\t\t\t\\\n static int stat_ ##slabname## realloc;\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static void init_ ##slabname## _with_stride(struct slabname *s,\t\t\\\n-\t\t\t\t\t    unsigned stride)\t\t\\\n+static inline void init_ ##slabname## _with_stride(struct slabname *s,\t\\\n+\t\t\t\t\t\t   unsigned stride)\t\\\n {\t\t\t\t\t\t\t\t\t\\\n \tunsigned int elem_size;\t\t\t\t\t\t\\\n \tif (!stride)\t\t\t\t\t\t\t\\\n@@ -58,12 +58,12 @@ struct slabname {\t\t\t\t\t\t\t\\\n \ts->slab = NULL;\t\t\t\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static void init_ ##slabname(struct slabname *s)\t\t\t\\\n+static inline void init_ ##slabname(struct slabname *s)\t\t\t\\\n {\t\t\t\t\t\t\t\t\t\\\n \tinit_ ##slabname## _with_stride(s, 1);\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static void clear_ ##slabname(struct slabname *s)\t\t\t\\\n+static inline void clear_ ##slabname(struct slabname *s)\t\t\\\n {\t\t\t\t\t\t\t\t\t\\\n \tint i;\t\t\t\t\t\t\t\t\\\n \tfor (i = 0; i < s->slab_count; i++)\t\t\t\t\\\n@@ -73,8 +73,8 @@ struct slabname {\t\t\t\t\t\t\t\\\n \ts->slab = NULL;\t\t\t\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static elemtype *slabname## _at(struct slabname *s,\t\t\t\\\n-\t\t\t\tconst struct commit *c)\t\t\t\\\n+static inline elemtype *slabname## _at(struct slabname *s,\t\t\\\n+\t\t\t\t       const struct commit *c)\t\t\\\n {\t\t\t\t\t\t\t\t\t\\\n \tint nth_slab, nth_slot;\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n@@ -94,8 +94,7 @@ struct slabname {\t\t\t\t\t\t\t\\\n \t\ts->slab[nth_slab] = xcalloc(s->slab_size,\t\t\\\n \t\t\t\t\t    sizeof(**s->slab) * s->stride);\t\t\\\n \treturn &s->slab[nth_slab][nth_slot * s->stride];\t\t\t\t\\\n-}\t\t\t\t\t\t\t\t\t\\\n-\t\t\t\t\t\t\t\t\t\\\n-static int stat_ ##slabname## realloc\n+}\n+\n \n #endif /* COMMIT_SLAB_H */\n-- \n1.8.5.rc3.397.g2a3acd5\n"},{"id":"231087","messageId":"xmqqy54cxohs.fsf@gitster.dls.corp.google.com","threadId":"35403","inReplyTo":"f4d1ff9f487f797da35faa86c72d11832903a50d.1385405977.git.tr@thomasrast.ch","subject":"Re: [PATCH 2/2] commit-slab: declare functions \"static inline\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-25T19:36:31Z","receivedAt":"2013-11-25T19:36:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <tr@thomasrast.ch> writes:\n\n> This shuts up compiler warnings about unused functions.\n\nThanks.\n\n> While there, also remove the redundant second declaration of\n> stat_##slabname##realloc.\n\nI think the latter was done very much deliberately to allow the\nusing code to say:\n\n\tdefine_commit_slab(name, type);\n\nby ending the macro with something that requires a terminating\nsemicolon.  If you just remove it, doesn't it break the compilation\nby forcing the expanded source to define a function\n\n\tslabname ## _at(...)\n        {\n        \t...\n\t};\n\nwith a trailing and undesired semicolon?\n\n>\n> Signed-off-by: Thomas Rast <tr@thomasrast.ch>\n> ---\n>  commit-slab.h | 17 ++++++++---------\n>  1 file changed, 8 insertions(+), 9 deletions(-)\n>\n> diff --git a/commit-slab.h b/commit-slab.h\n> index d77aaea..d5c353e 100644\n> --- a/commit-slab.h\n> +++ b/commit-slab.h\n> @@ -45,8 +45,8 @@ struct slabname {\t\t\t\t\t\t\t\\\n>  };\t\t\t\t\t\t\t\t\t\\\n>  static int stat_ ##slabname## realloc;\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n> -static void init_ ##slabname## _with_stride(struct slabname *s,\t\t\\\n> -\t\t\t\t\t    unsigned stride)\t\t\\\n> +static inline void init_ ##slabname## _with_stride(struct slabname *s,\t\\\n> +\t\t\t\t\t\t   unsigned stride)\t\\\n>  {\t\t\t\t\t\t\t\t\t\\\n>  \tunsigned int elem_size;\t\t\t\t\t\t\\\n>  \tif (!stride)\t\t\t\t\t\t\t\\\n> @@ -58,12 +58,12 @@ struct slabname {\t\t\t\t\t\t\t\\\n>  \ts->slab = NULL;\t\t\t\t\t\t\t\\\n>  }\t\t\t\t\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n> -static void init_ ##slabname(struct slabname *s)\t\t\t\\\n> +static inline void init_ ##slabname(struct slabname *s)\t\t\t\\\n>  {\t\t\t\t\t\t\t\t\t\\\n>  \tinit_ ##slabname## _with_stride(s, 1);\t\t\t\t\\\n>  }\t\t\t\t\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n> -static void clear_ ##slabname(struct slabname *s)\t\t\t\\\n> +static inline void clear_ ##slabname(struct slabname *s)\t\t\\\n>  {\t\t\t\t\t\t\t\t\t\\\n>  \tint i;\t\t\t\t\t\t\t\t\\\n>  \tfor (i = 0; i < s->slab_count; i++)\t\t\t\t\\\n> @@ -73,8 +73,8 @@ struct slabname {\t\t\t\t\t\t\t\\\n>  \ts->slab = NULL;\t\t\t\t\t\t\t\\\n>  }\t\t\t\t\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n> -static elemtype *slabname## _at(struct slabname *s,\t\t\t\\\n> -\t\t\t\tconst struct commit *c)\t\t\t\\\n> +static inline elemtype *slabname## _at(struct slabname *s,\t\t\\\n> +\t\t\t\t       const struct commit *c)\t\t\\\n>  {\t\t\t\t\t\t\t\t\t\\\n>  \tint nth_slab, nth_slot;\t\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n> @@ -94,8 +94,7 @@ struct slabname {\t\t\t\t\t\t\t\\\n>  \t\ts->slab[nth_slab] = xcalloc(s->slab_size,\t\t\\\n>  \t\t\t\t\t    sizeof(**s->slab) * s->stride);\t\t\\\n>  \treturn &s->slab[nth_slab][nth_slot * s->stride];\t\t\t\t\\\n> -}\t\t\t\t\t\t\t\t\t\\\n> -\t\t\t\t\t\t\t\t\t\\\n> -static int stat_ ##slabname## realloc\n> +}\n> +\n>  \n>  #endif /* COMMIT_SLAB_H */\n"},{"id":"231088","messageId":"878uwc2r7c.fsf@thomasrast.ch","threadId":"35403","inReplyTo":"xmqqy54cxohs.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] commit-slab: declare functions \"static inline\"","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-25T19:53:43Z","receivedAt":"2013-11-25T19:53:43Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Thomas Rast <tr@thomasrast.ch> writes:\n>\n>> This shuts up compiler warnings about unused functions.\n>\n> Thanks.\n>\n>> While there, also remove the redundant second declaration of\n>> stat_##slabname##realloc.\n>\n> I think the latter was done very much deliberately to allow the\n> using code to say:\n>\n> \tdefine_commit_slab(name, type);\n>\n> by ending the macro with something that requires a terminating\n> semicolon.  If you just remove it, doesn't it break the compilation\n> by forcing the expanded source to define a function\n>\n> \tslabname ## _at(...)\n>         {\n>         \t...\n> \t};\n>\n> with a trailing and undesired semicolon?\n\nOooh.  The sudden enlightenment.\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"231089","messageId":"89b534b37f5689a675f0f97d3627a0668ce2a71d.1385409724.git.tr@thomasrast.ch","threadId":"35403","inReplyTo":"878uwc2r7c.fsf@thomasrast.ch","subject":"[PATCH v2] commit-slab: declare functions \"static inline\"","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-25T20:04:08Z","receivedAt":"2013-11-25T20:04:08Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"This shuts up compiler warnings about unused functions.  No such\nwarnings are currently triggered, but if someone were to actually use\ninit_NAME_with_stride() as documented, they would get a warning about\ninit_NAME() being unused.\n\nWhile there, write a comment about why we need two declarations of the\nsame variable.\n\nSigned-off-by: Thomas Rast <tr@thomasrast.ch>\n---\n\nHere's a version that has a fat comment instead of the removal.\n\nAlso, since I was rerolling anyway I put a reason why we need this.\nIn the original motivation I actually created more functions\nafterwards, which made it more convincing, but the problem already\nexists.\n\n\n commit-slab.h | 24 ++++++++++++++++++------\n 1 file changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex d77aaea..21d54f1 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -45,8 +45,8 @@ struct slabname {\t\t\t\t\t\t\t\\\n };\t\t\t\t\t\t\t\t\t\\\n static int stat_ ##slabname## realloc;\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static void init_ ##slabname## _with_stride(struct slabname *s,\t\t\\\n-\t\t\t\t\t    unsigned stride)\t\t\\\n+static inline void init_ ##slabname## _with_stride(struct slabname *s,\t\\\n+\t\t\t\t\t\t   unsigned stride)\t\\\n {\t\t\t\t\t\t\t\t\t\\\n \tunsigned int elem_size;\t\t\t\t\t\t\\\n \tif (!stride)\t\t\t\t\t\t\t\\\n@@ -58,12 +58,12 @@ struct slabname {\t\t\t\t\t\t\t\\\n \ts->slab = NULL;\t\t\t\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static void init_ ##slabname(struct slabname *s)\t\t\t\\\n+static inline void init_ ##slabname(struct slabname *s)\t\t\t\\\n {\t\t\t\t\t\t\t\t\t\\\n \tinit_ ##slabname## _with_stride(s, 1);\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static void clear_ ##slabname(struct slabname *s)\t\t\t\\\n+static inline void clear_ ##slabname(struct slabname *s)\t\t\\\n {\t\t\t\t\t\t\t\t\t\\\n \tint i;\t\t\t\t\t\t\t\t\\\n \tfor (i = 0; i < s->slab_count; i++)\t\t\t\t\\\n@@ -73,8 +73,8 @@ struct slabname {\t\t\t\t\t\t\t\\\n \ts->slab = NULL;\t\t\t\t\t\t\t\\\n }\t\t\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n-static elemtype *slabname## _at(struct slabname *s,\t\t\t\\\n-\t\t\t\tconst struct commit *c)\t\t\t\\\n+static inline elemtype *slabname## _at(struct slabname *s,\t\t\\\n+\t\t\t\t       const struct commit *c)\t\t\\\n {\t\t\t\t\t\t\t\t\t\\\n \tint nth_slab, nth_slot;\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n@@ -98,4 +98,16 @@ struct slabname {\t\t\t\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n static int stat_ ##slabname## realloc\n \n+/*\n+ * Note that this seemingly redundant second declaration is required\n+ * to allow a terminating semicolon, which makes instantiations look\n+ * like function declarations.  I.e., the expansion of\n+ *\n+ *    define_commit_slab(indegree, int);\n+ *\n+ * ends in 'static int stat_indegreerealloc;'.  This would otherwise\n+ * be a syntax error according (at least) to ISO C.  It's hard to\n+ * catch because GCC silently parses it by default.\n+ */\n+\n #endif /* COMMIT_SLAB_H */\n-- \n1.8.5.rc2.355.g6969a19\n"},{"id":"231091","messageId":"20131125201200.GN4212@google.com","threadId":"35403","inReplyTo":"89b534b37f5689a675f0f97d3627a0668ce2a71d.1385409724.git.tr@thomasrast.ch","subject":"Re: [PATCH v2] commit-slab: declare functions \"static inline\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-11-25T20:12:00Z","receivedAt":"2013-11-25T20:12:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Rast wrote:\n\n> This shuts up compiler warnings about unused functions.\n\nIf that is the only goal, I think it would be cleaner to use\n\n\t#define MAYBE_UNUSED __attribute__((__unused__))\n\n\tstatic MAYBE_UNUSED void init_ ...\n\nlike was done in the vcs-svn/ directory until cba3546 (drop obj_pool,\n2010-12-13) et al.\n\nI haven't thought carefully about whether encouraging inlining here\n(or encouraging the reader to think of these functions as inline) is a\ngood or bad change.\n\n[...]\n> @@ -98,4 +98,16 @@ struct slabname {\t\t\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n>  static int stat_ ##slabname## realloc\n>  \n> +/*\n> + * Note that this seemingly redundant second declaration is required\n> + * to allow a terminating semicolon, which makes instantiations look\n> + * like function declarations.  I.e., the expansion of\n\nMicronit: this reads more clearly without the \"Note that\".  That is,\nthe comment can get the reader's attention more easily by going right\ninto what it is about to say without asking for the reader's\nattention:\n\n\t/*\n\t * This seemingly redundant second declaration is required to ...\n\nThanks,\nJonathan\n"},{"id":"231092","messageId":"87wqjw1bm5.fsf@thomasrast.ch","threadId":"35403","inReplyTo":"20131125201200.GN4212@google.com","subject":"Re: [PATCH v2] commit-slab: declare functions \"static inline\"","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-25T20:15:46Z","receivedAt":"2013-11-25T20:15:46Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Thomas Rast wrote:\n>\n>> This shuts up compiler warnings about unused functions.\n>\n> If that is the only goal, I think it would be cleaner to use\n>\n> \t#define MAYBE_UNUSED __attribute__((__unused__))\n>\n> \tstatic MAYBE_UNUSED void init_ ...\n>\n> like was done in the vcs-svn/ directory until cba3546 (drop obj_pool,\n> 2010-12-13) et al.\n>\n> I haven't thought carefully about whether encouraging inlining here\n> (or encouraging the reader to think of these functions as inline) is a\n> good or bad change.\n\nHmm.\n\nI actually had this idea after seeing the same trick in khash.h.  Is\n__atribute__((__unused__)) universal?  If so, maybe we could apply the\nsame also to khash?  If not, I'd rather go with the inline.\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"231094","messageId":"xmqqmwksxmap.fsf@gitster.dls.corp.google.com","threadId":"35403","inReplyTo":"89b534b37f5689a675f0f97d3627a0668ce2a71d.1385409724.git.tr@thomasrast.ch","subject":"Re: [PATCH v2] commit-slab: declare functions \"static inline\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-25T20:23:58Z","receivedAt":"2013-11-25T20:23:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <tr@thomasrast.ch> writes:\n\n> Here's a version that has a fat comment instead of the removal.\n>\n> Also, since I was rerolling anyway I put a reason why we need this.\n> In the original motivation I actually created more functions\n> afterwards, which made it more convincing, but the problem already\n> exists.\n\nThanks.\n\nI considered the bottom one the real declaration (with the top one a\nforward declaration we need to make the result compile), by the way,\nso there may be no redundancy anywhere ;-)\n\n\n\n>  commit-slab.h | 24 ++++++++++++++++++------\n>  1 file changed, 18 insertions(+), 6 deletions(-)\n>\n> diff --git a/commit-slab.h b/commit-slab.h\n> index d77aaea..21d54f1 100644\n> --- a/commit-slab.h\n> +++ b/commit-slab.h\n> @@ -45,8 +45,8 @@ struct slabname {\t\t\t\t\t\t\t\\\n>  };\t\t\t\t\t\t\t\t\t\\\n>  static int stat_ ##slabname## realloc;\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n> -static void init_ ##slabname## _with_stride(struct slabname *s,\t\t\\\n> -\t\t\t\t\t    unsigned stride)\t\t\\\n> +static inline void init_ ##slabname## _with_stride(struct slabname *s,\t\\\n> +\t\t\t\t\t\t   unsigned stride)\t\\\n>  {\t\t\t\t\t\t\t\t\t\\\n>  \tunsigned int elem_size;\t\t\t\t\t\t\\\n>  \tif (!stride)\t\t\t\t\t\t\t\\\n> @@ -58,12 +58,12 @@ struct slabname {\t\t\t\t\t\t\t\\\n>  \ts->slab = NULL;\t\t\t\t\t\t\t\\\n>  }\t\t\t\t\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n> -static void init_ ##slabname(struct slabname *s)\t\t\t\\\n> +static inline void init_ ##slabname(struct slabname *s)\t\t\t\\\n>  {\t\t\t\t\t\t\t\t\t\\\n>  \tinit_ ##slabname## _with_stride(s, 1);\t\t\t\t\\\n>  }\t\t\t\t\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n> -static void clear_ ##slabname(struct slabname *s)\t\t\t\\\n> +static inline void clear_ ##slabname(struct slabname *s)\t\t\\\n>  {\t\t\t\t\t\t\t\t\t\\\n>  \tint i;\t\t\t\t\t\t\t\t\\\n>  \tfor (i = 0; i < s->slab_count; i++)\t\t\t\t\\\n> @@ -73,8 +73,8 @@ struct slabname {\t\t\t\t\t\t\t\\\n>  \ts->slab = NULL;\t\t\t\t\t\t\t\\\n>  }\t\t\t\t\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n> -static elemtype *slabname## _at(struct slabname *s,\t\t\t\\\n> -\t\t\t\tconst struct commit *c)\t\t\t\\\n> +static inline elemtype *slabname## _at(struct slabname *s,\t\t\\\n> +\t\t\t\t       const struct commit *c)\t\t\\\n>  {\t\t\t\t\t\t\t\t\t\\\n>  \tint nth_slab, nth_slot;\t\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n> @@ -98,4 +98,16 @@ struct slabname {\t\t\t\t\t\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n>  static int stat_ ##slabname## realloc\n>  \n> +/*\n> + * Note that this seemingly redundant second declaration is required\n> + * to allow a terminating semicolon, which makes instantiations look\n> + * like function declarations.  I.e., the expansion of\n> + *\n> + *    define_commit_slab(indegree, int);\n> + *\n> + * ends in 'static int stat_indegreerealloc;'.  This would otherwise\n> + * be a syntax error according (at least) to ISO C.  It's hard to\n> + * catch because GCC silently parses it by default.\n> + */\n> +\n>  #endif /* COMMIT_SLAB_H */\n"},{"id":"231095","messageId":"20131125202409.GO4212@google.com","threadId":"35403","inReplyTo":"7f773c5c5ea16b19840f67ba99961be132940d32.1385405977.git.tr@thomasrast.ch","subject":"Re: [PATCH 1/2] commit-slab: document clear_$slabname()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-11-25T20:24:09Z","receivedAt":"2013-11-25T20:24:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Rast wrote:\n\n> The clear_$slabname() function was only documented by source code so\n> far.  Write something about it.\n\nGood idea.\n\n[...]\n> --- a/commit-slab.h\n> +++ b/commit-slab.h\n> @@ -24,6 +24,10 @@\n>   *   to each commit. 'stride' specifies how big each array is.  The slab\n>   *   that id initialied by the variant without \"_with_stride\" associates\n>   *   each commit with an array of one integer.\n> + *\n> + * - void clear_indegree(struct indegree *);\n> + *\n> + *   Free the slab's data structures.\n\nTense shift (previous descriptions were in the present tense, while\nthis one is in the imperative).\n\nMore importantly, this doesn't answer the questions I'd have if I were\nin a hurry, which are what exactly is being freed (has the slab taken\nownership of any memory from the user, e.g. when elemtype is a\npointer?) and whether the slab needs to be init_ ed again.\n\nMaybe something like the following would work?\n\n\t- void clear_indegree(struct indegree *);\n\n\t  Empties the slab.  The slab can be reused with the same\n\t  stride without calling init_indegree again or can be\n\t  reconfigured to a different stride by calling\n\t  init_indegree_with_stride.\n\n\t  Call this function before the slab falls out of scope to\n\t  avoid leaking memory.\n\nThanks,\nJonathan\n"},{"id":"231096","messageId":"xmqqiovgxluu.fsf@gitster.dls.corp.google.com","threadId":"35403","inReplyTo":"20131125201200.GN4212@google.com","subject":"Re: [PATCH v2] commit-slab: declare functions \"static inline\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-25T20:33:29Z","receivedAt":"2013-11-25T20:33:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Thomas Rast wrote:\n>\n>> This shuts up compiler warnings about unused functions.\n>\n> If that is the only goal, I think it would be cleaner to use\n>\n> \t#define MAYBE_UNUSED __attribute__((__unused__))\n>\n> \tstatic MAYBE_UNUSED void init_ ...\n>\n> like was done in the vcs-svn/ directory until cba3546 (drop obj_pool,\n> 2010-12-13) et al.\n>\n> I haven't thought carefully about whether encouraging inlining here\n> (or encouraging the reader to think of these functions as inline) is a\n> good or bad change.\n>\n> [...]\n>> @@ -98,4 +98,16 @@ struct slabname {\t\t\t\t\t\t\t\\\n>>  \t\t\t\t\t\t\t\t\t\\\n>>  static int stat_ ##slabname## realloc\n>>  \n>> +/*\n>> + * Note that this seemingly redundant second declaration is required\n>> + * to allow a terminating semicolon, which makes instantiations look\n>> + * like function declarations.  I.e., the expansion of\n>\n> Micronit: this reads more clearly without the \"Note that\".  That is,\n> the comment can get the reader's attention more easily by going right\n> into what it is about to say without asking for the reader's\n> attention:\n>\n> \t/*\n> \t * This seemingly redundant second declaration is required to ...\n\nHmm, both of these are good points.\n\nThanks.\n"},{"id":"231098","messageId":"20131125203557.GP4212@google.com","threadId":"35403","inReplyTo":"87wqjw1bm5.fsf@thomasrast.ch","subject":"Re: [PATCH v2] commit-slab: declare functions \"static inline\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-11-25T20:35:57Z","receivedAt":"2013-11-25T20:35:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Rast wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>> Thomas Rast wrote:\n\n>>> This shuts up compiler warnings about unused functions.\n>>\n>> If that is the only goal, I think it would be cleaner to use\n>>\n>> \t#define MAYBE_UNUSED __attribute__((__unused__))\n>>\n>> \tstatic MAYBE_UNUSED void init_ ...\n>>\n>> like was done in the vcs-svn/ directory until cba3546 (drop obj_pool,\n>> 2010-12-13) et al.\n>>\n>> I haven't thought carefully about whether encouraging inlining here\n>> (or encouraging the reader to think of these functions as inline) is a\n>> good or bad change.\n>\n> Hmm.\n>\n> I actually had this idea after seeing the same trick in khash.h.  Is\n> __atribute__((__unused__)) universal?  If so, maybe we could apply the\n> same also to khash?  If not, I'd rather go with the inline.\n\nThe khash functions are very small, so it very well may make sense for\nthem to be inline.\n\ngit-compat-util.h (or compat/msvc.h) defines __attribute__(x) to an\nempty sequence of tokens except on HP C and gcc.  Attribute unused has\nexisted at least since GCC 2.95.\n\nUnfortunately HP C doesn't support attribute __unused__. :(\nhttp://h21007.www2.hp.com/portal/download/files/unprot/aCxx/Online_Help/pragmas.htm#Attributes\n\nOn the bright side, it would be easy to work around using a\nconditional definition of MAYBE_UNUSED for the sake of HP C.  From\nAugust 2010 until March 2011 nobody noticed.\n\nJonathan\n"},{"id":"231099","messageId":"xmqqa9gsxll5.fsf@gitster.dls.corp.google.com","threadId":"35403","inReplyTo":"7f773c5c5ea16b19840f67ba99961be132940d32.1385405977.git.tr@thomasrast.ch","subject":"Re: [PATCH 1/2] commit-slab: document clear_$slabname()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-25T20:39:18Z","receivedAt":"2013-11-25T20:39:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <tr@thomasrast.ch> writes:\n\n> The clear_$slabname() function was only documented by source code so\n> far.  Write something about it.\n>\n> Signed-off-by: Thomas Rast <tr@thomasrast.ch>\n> ---\n>  commit-slab.h | 4 ++++\n>  1 file changed, 4 insertions(+)\n>\n> diff --git a/commit-slab.h b/commit-slab.h\n> index d4c8286..d77aaea 100644\n> --- a/commit-slab.h\n> +++ b/commit-slab.h\n> @@ -24,6 +24,10 @@\n>   *   to each commit. 'stride' specifies how big each array is.  The slab\n>   *   that id initialied by the variant without \"_with_stride\" associates\n\nIs that \"id\" a typo for \"is\"?\n\n>   *   each commit with an array of one integer.\n> + *\n> + * - void clear_indegree(struct indegree *);\n> + *\n> + *   Free the slab's data structures.\n>   */\n>  \n>  /* allocate ~512kB at once, allowing for malloc overhead */\n"},{"id":"231163","messageId":"CAPig+cRmNsGLr1=xrWCgs8DxJ0QVhn1na=0UtjwVswKvPGdqDw@mail.gmail.com","threadId":"35403","inReplyTo":"xmqqa9gsxll5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] commit-slab: document clear_$slabname()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-11-27T10:39:17Z","receivedAt":"2013-11-27T10:39:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 25, 2013 at 3:39 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Thomas Rast <tr@thomasrast.ch> writes:\n>\n>> The clear_$slabname() function was only documented by source code so\n>> far.  Write something about it.\n>>\n>> Signed-off-by: Thomas Rast <tr@thomasrast.ch>\n>> ---\n>>  commit-slab.h | 4 ++++\n>>  1 file changed, 4 insertions(+)\n>>\n>> diff --git a/commit-slab.h b/commit-slab.h\n>> index d4c8286..d77aaea 100644\n>> --- a/commit-slab.h\n>> +++ b/commit-slab.h\n>> @@ -24,6 +24,10 @@\n>>   *   to each commit. 'stride' specifies how big each array is.  The slab\n>>   *   that id initialied by the variant without \"_with_stride\" associates\n>\n> Is that \"id\" a typo for \"is\"?\n\nAnd, s/initialied/initialized/\n\n>>   *   each commit with an array of one integer.\n>> + *\n>> + * - void clear_indegree(struct indegree *);\n>> + *\n>> + *   Free the slab's data structures.\n>>   */\n>>\n>>  /* allocate ~512kB at once, allowing for malloc overhead */\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"231278","messageId":"87txevdms2.fsf@thomasrast.ch","threadId":"35403","inReplyTo":"20131125202409.GO4212@google.com","subject":"Re: [PATCH 1/2] commit-slab: document clear_$slabname()","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-29T19:35:09Z","receivedAt":"2013-11-29T19:35:09Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Thomas Rast wrote:\n>\n>> + *\n>> + * - void clear_indegree(struct indegree *);\n>> + *\n>> + *   Free the slab's data structures.\n>\n> Tense shift (previous descriptions were in the present tense, while\n> this one is in the imperative).\n>\n> More importantly, this doesn't answer the questions I'd have if I were\n> in a hurry, which are what exactly is being freed (has the slab taken\n> ownership of any memory from the user, e.g. when elemtype is a\n> pointer?) and whether the slab needs to be init_ ed again.\n>\n> Maybe something like the following would work?\n[...]\n\nOk, I see that while I was procrastinating, you sorted this out and\nJunio merged it to next.\n\nThanks, both.\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"231327","messageId":"CACsJy8CnxvPRwC_xXgBNF_JEmkpfnk=faMwOWtkJOFU-18aHgA@mail.gmail.com","threadId":"35403","inReplyTo":"f4d1ff9f487f797da35faa86c72d11832903a50d.1385405977.git.tr@thomasrast.ch","subject":"Re: [PATCH 2/2] commit-slab: declare functions \"static inline\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-12-01T10:36:31Z","receivedAt":"2013-12-01T10:36:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Thomas,\n\nAs you're touching this, perhaps you coud fix this line in slabname_at() too?\n\ns->slab = xrealloc(s->slab, (nth_slab + 1) * sizeof(s->slab));\n\nI think it should be sizeof(*s->slab), not sizeof(s->slab), even\nthough the end result is the same.\n-- \nDuy\n"},{"id":"231337","messageId":"87siucpalo.fsf_-_@linux-1gf2.Speedport_W723_V_Typ_A_1_00_098","threadId":"35403","inReplyTo":"CACsJy8CnxvPRwC_xXgBNF_JEmkpfnk=faMwOWtkJOFU-18aHgA@mail.gmail.com","subject":"[PATCH] commit-slab: sizeof() the right type in xrealloc","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-12-01T20:41:55Z","receivedAt":"2013-12-01T20:41:55Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"When allocating the slab, the code accidentally computed the array\nsize from s->slab (an elemtype**).  The slab is an array of elemtype*,\nhowever, so we should take the size of *s->slab.\n\nNoticed-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Thomas Rast <tr@thomasrast.ch>\n---\n[I hope this comes through clean.  git-send-email is currently broken\nfor me, and I'm still investigating, so I have to kludge around it.]\n\nI browsed around for a while, and couldn't find out whether any\narchitecture actually has any hope of running git (i.e. is at least\nmostly POSIX conformant) but still violates the assumption that all\npointers[*] are the same size.\n\nThe comp.lang.c FAQ has some interesting examples of wildly different\npointer representations at:\n\n  http://c-faq.com/null/machexamp.html\n\nConsider the cases mentioned there where void* and char* have\ndifferent representations from, e.g., int*.  Then if elemtype is char,\nthe slab will be too small before this patch.\n\nBut I have no idea if any of those are POSIXish.\n\nOne interesting, though orthogonal, tidbit is that POSIX actually\nrequires _function_ pointers to have the same representation as void*.\nFrom the specification of dlsym(), which depends on this to be able to\nreturn function pointers:\n\nRATIONALE\n  [...] Indeed, the ISO C standard does not require that an object of\n  type void * can hold a pointer to a function. Implementations\n  supporting the XSI extension, however, do require that an object of\n  type void * can hold a pointer to a function.\n\nThank god for POSIX.  So much craziness averted.\n\n\n[*] Note that C++ method pointers are yet another story.  This only\napplies to the kinds of pointers that C supports.\n\n\n commit-slab.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex d068e2d..cc114b5 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -91,7 +91,7 @@ struct slabname {\t\t\t\t\t\t\t\\\n \tif (s->slab_count <= nth_slab) {\t\t\t\t\\\n \t\tint i;\t\t\t\t\t\t\t\\\n \t\ts->slab = xrealloc(s->slab,\t\t\t\t\\\n-\t\t\t\t   (nth_slab + 1) * sizeof(s->slab));\t\\\n+\t\t\t\t   (nth_slab + 1) * sizeof(*s->slab));\t\\\n \t\tstat_ ##slabname## realloc++;\t\t\t\t\\\n \t\tfor (i = s->slab_count; i <= nth_slab; i++)\t\t\\\n \t\t\ts->slab[i] = NULL;\t\t\t\t\\\n-- \n1.8.5.427.g6d3141d\n"},{"id":"231373","messageId":"20131202153513.GA24202@sigill.intra.peff.net","threadId":"35403","inReplyTo":"87siucpalo.fsf_-_@linux-1gf2.Speedport_W723_V_Typ_A_1_00_098","subject":"Re: [PATCH] commit-slab: sizeof() the right type in xrealloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-02T15:35:13Z","receivedAt":"2013-12-02T15:35:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 01, 2013 at 09:41:55PM +0100, Thomas Rast wrote:\n\n> When allocating the slab, the code accidentally computed the array\n> size from s->slab (an elemtype**).  The slab is an array of elemtype*,\n> however, so we should take the size of *s->slab.\n\nLooks obviously correct.\n\n> I browsed around for a while, and couldn't find out whether any\n> architecture actually has any hope of running git (i.e. is at least\n> mostly POSIX conformant) but still violates the assumption that all\n> pointers[*] are the same size.\n> \n> The comp.lang.c FAQ has some interesting examples of wildly different\n> pointer representations at:\n> \n>   http://c-faq.com/null/machexamp.html\n\nNote that most of those examples are not different sizes, but rather\ndifferent null-pointer representations. We are already grossly out of\ncompliance with the standard there, as we typically use memset() to\nzero-out structs with pointers.\n\n-Peff\n"}]}