patch, 2 partscommit-slab cleanups

> --- a/commit-slab.h
> +++ b/commit-slab.h
> @@ -24,6 +24,10 @@
>   *   to each commit. 'stride' specifies how big each array is.  The slab
>   *   that id initialied by the variant without "_with_stride" associates
>   *   each commit with an array of one integer.
> + *
> + * - void clear_indegree(struct indegree *);
> + *
> + *   Free the slab's data structures.

> Thomas Rast wrote:
>
>> + *
>> + * - void clear_indegree(struct indegree *);
>> + *
>> + *   Free the slab's data structures.
>
> Tense shift (previous descriptions were in the present tense, while
> this one is in the imperative).
>
> More importantly, this doesn't answer the questions I'd have if I were
> in a hurry, which are what exactly is being freed (has the slab taken
> ownership of any memory from the user, e.g. when elemtype is a
> pointer?) and whether the slab needs to be init_ ed again.
>
> Maybe something like the following would work?

> The clear_$slabname() function was only documented by source code so
> far.  Write something about it.
>
> Signed-off-by: Thomas Rast <tr@thomasrast.ch>
> ---
>  commit-slab.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/commit-slab.h b/commit-slab.h
> index d4c8286..d77aaea 100644
> --- a/commit-slab.h
> +++ b/commit-slab.h
> @@ -24,6 +24,10 @@
>   *   to each commit. 'stride' specifies how big each array is.  The slab
>   *   that id initialied by the variant without "_with_stride" associates

>   *   each commit with an array of one integer.
> + *
> + * - void clear_indegree(struct indegree *);
> + *
> + *   Free the slab's data structures.
>   */
>  
>  /* allocate ~512kB at once, allowing for malloc overhead */

> Thomas Rast <tr@thomasrast.ch> writes:
>
>> The clear_$slabname() function was only documented by source code so
>> far.  Write something about it.
>>
>> Signed-off-by: Thomas Rast <tr@thomasrast.ch>
>> ---
>>  commit-slab.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/commit-slab.h b/commit-slab.h
>> index d4c8286..d77aaea 100644
>> --- a/commit-slab.h
>> +++ b/commit-slab.h
>> @@ -24,6 +24,10 @@
>>   *   to each commit. 'stride' specifies how big each array is.  The slab
>>   *   that id initialied by the variant without "_with_stride" associates
>
> Is that "id" a typo for "is"?

>>   *   each commit with an array of one integer.
>> + *
>> + * - void clear_indegree(struct indegree *);
>> + *
>> + *   Free the slab's data structures.
>>   */
>>
>>  /* allocate ~512kB at once, allowing for malloc overhead */
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

>
> Signed-off-by: Thomas Rast <tr@thomasrast.ch>
> ---
>  commit-slab.h | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/commit-slab.h b/commit-slab.h
> index d77aaea..d5c353e 100644
> --- a/commit-slab.h
> +++ b/commit-slab.h
> @@ -45,8 +45,8 @@ struct slabname {							\
>  };									\
>  static int stat_ ##slabname## realloc;					\
>  									\
> -static void init_ ##slabname## _with_stride(struct slabname *s,		\
> -					    unsigned stride)		\
> +static inline void init_ ##slabname## _with_stride(struct slabname *s,	\
> +						   unsigned stride)	\
>  {									\
>  	unsigned int elem_size;						\
>  	if (!stride)							\
> @@ -58,12 +58,12 @@ struct slabname {							\
>  	s->slab = NULL;							\
>  }									\
>  									\
> -static void init_ ##slabname(struct slabname *s)			\
> +static inline void init_ ##slabname(struct slabname *s)			\
>  {									\
>  	init_ ##slabname## _with_stride(s, 1);				\
>  }									\
>  									\
> -static void clear_ ##slabname(struct slabname *s)			\
> +static inline void clear_ ##slabname(struct slabname *s)		\
>  {									\
>  	int i;								\
>  	for (i = 0; i < s->slab_count; i++)				\
> @@ -73,8 +73,8 @@ struct slabname {							\
>  	s->slab = NULL;							\
>  }									\
>  									\
> -static elemtype *slabname## _at(struct slabname *s,			\
> -				const struct commit *c)			\
> +static inline elemtype *slabname## _at(struct slabname *s,		\
> +				       const struct commit *c)		\
>  {									\
>  	int nth_slab, nth_slot;						\
>  									\
> @@ -94,8 +94,7 @@ struct slabname {							\
>  		s->slab[nth_slab] = xcalloc(s->slab_size,		\
>  					    sizeof(**s->slab) * s->stride);		\
>  	return &s->slab[nth_slab][nth_slot * s->stride];				\
> -}									\
> -									\
> -static int stat_ ##slabname## realloc
> +}
> +
>  
>  #endif /* COMMIT_SLAB_H */

> Thomas Rast <tr@thomasrast.ch> writes:
>
>> This shuts up compiler warnings about unused functions.
>
> Thanks.
>
>> While there, also remove the redundant second declaration of
>> stat_##slabname##realloc.
>
> I think the latter was done very much deliberately to allow the
> using code to say:
>
> 	define_commit_slab(name, type);
>
> by ending the macro with something that requires a terminating
> semicolon.  If you just remove it, doesn't it break the compilation
> by forcing the expanded source to define a function
>
> 	slabname ## _at(...)
>         {
>         	...
> 	};
>
> with a trailing and undesired semicolon?

> @@ -98,4 +98,16 @@ struct slabname {							\
>  									\
>  static int stat_ ##slabname## realloc
>  
> +/*
> + * Note that this seemingly redundant second declaration is required
> + * to allow a terminating semicolon, which makes instantiations look
> + * like function declarations.  I.e., the expansion of

> Thomas Rast wrote:
>
>> This shuts up compiler warnings about unused functions.
>
> If that is the only goal, I think it would be cleaner to use
>
> 	#define MAYBE_UNUSED __attribute__((__unused__))
>
> 	static MAYBE_UNUSED void init_ ...
>
> like was done in the vcs-svn/ directory until cba3546 (drop obj_pool,
> 2010-12-13) et al.
>
> I haven't thought carefully about whether encouraging inlining here
> (or encouraging the reader to think of these functions as inline) is a
> good or bad change.

>>> This shuts up compiler warnings about unused functions.
>>
>> If that is the only goal, I think it would be cleaner to use
>>
>> 	#define MAYBE_UNUSED __attribute__((__unused__))
>>
>> 	static MAYBE_UNUSED void init_ ...
>>
>> like was done in the vcs-svn/ directory until cba3546 (drop obj_pool,
>> 2010-12-13) et al.
>>
>> I haven't thought carefully about whether encouraging inlining here
>> (or encouraging the reader to think of these functions as inline) is a
>> good or bad change.
>
> Hmm.
>
> I actually had this idea after seeing the same trick in khash.h.  Is
> __atribute__((__unused__)) universal?  If so, maybe we could apply the
> same also to khash?  If not, I'd rather go with the inline.

> Thomas Rast wrote:
>
>> This shuts up compiler warnings about unused functions.
>
> If that is the only goal, I think it would be cleaner to use
>
> 	#define MAYBE_UNUSED __attribute__((__unused__))
>
> 	static MAYBE_UNUSED void init_ ...
>
> like was done in the vcs-svn/ directory until cba3546 (drop obj_pool,
> 2010-12-13) et al.
>
> I haven't thought carefully about whether encouraging inlining here
> (or encouraging the reader to think of these functions as inline) is a
> good or bad change.
>
> [...]
>> @@ -98,4 +98,16 @@ struct slabname {							\
>>  									\
>>  static int stat_ ##slabname## realloc
>>  
>> +/*
>> + * Note that this seemingly redundant second declaration is required
>> + * to allow a terminating semicolon, which makes instantiations look
>> + * like function declarations.  I.e., the expansion of
>
> Micronit: this reads more clearly without the "Note that".  That is,
> the comment can get the reader's attention more easily by going right
> into what it is about to say without asking for the reader's
> attention:
>
> 	/*
> 	 * This seemingly redundant second declaration is required to ...

> Here's a version that has a fat comment instead of the removal.
>
> Also, since I was rerolling anyway I put a reason why we need this.
> In the original motivation I actually created more functions
> afterwards, which made it more convincing, but the problem already
> exists.

>  commit-slab.h | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/commit-slab.h b/commit-slab.h
> index d77aaea..21d54f1 100644
> --- a/commit-slab.h
> +++ b/commit-slab.h
> @@ -45,8 +45,8 @@ struct slabname {							\
>  };									\
>  static int stat_ ##slabname## realloc;					\
>  									\
> -static void init_ ##slabname## _with_stride(struct slabname *s,		\
> -					    unsigned stride)		\
> +static inline void init_ ##slabname## _with_stride(struct slabname *s,	\
> +						   unsigned stride)	\
>  {									\
>  	unsigned int elem_size;						\
>  	if (!stride)							\
> @@ -58,12 +58,12 @@ struct slabname {							\
>  	s->slab = NULL;							\
>  }									\
>  									\
> -static void init_ ##slabname(struct slabname *s)			\
> +static inline void init_ ##slabname(struct slabname *s)			\
>  {									\
>  	init_ ##slabname## _with_stride(s, 1);				\
>  }									\
>  									\
> -static void clear_ ##slabname(struct slabname *s)			\
> +static inline void clear_ ##slabname(struct slabname *s)		\
>  {									\
>  	int i;								\
>  	for (i = 0; i < s->slab_count; i++)				\
> @@ -73,8 +73,8 @@ struct slabname {							\
>  	s->slab = NULL;							\
>  }									\
>  									\
> -static elemtype *slabname## _at(struct slabname *s,			\
> -				const struct commit *c)			\
> +static inline elemtype *slabname## _at(struct slabname *s,		\
> +				       const struct commit *c)		\
>  {									\
>  	int nth_slab, nth_slot;						\
>  									\
> @@ -98,4 +98,16 @@ struct slabname {							\
>  									\
>  static int stat_ ##slabname## realloc
>  
> +/*
> + * Note that this seemingly redundant second declaration is required
> + * to allow a terminating semicolon, which makes instantiations look
> + * like function declarations.  I.e., the expansion of
> + *
> + *    define_commit_slab(indegree, int);
> + *
> + * ends in 'static int stat_indegreerealloc;'.  This would otherwise
> + * be a syntax error according (at least) to ISO C.  It's hard to
> + * catch because GCC silently parses it by default.
> + */
> +
>  #endif /* COMMIT_SLAB_H */

> I browsed around for a while, and couldn't find out whether any
> architecture actually has any hope of running git (i.e. is at least
> mostly POSIX conformant) but still violates the assumption that all
> pointers[*] are the same size.
> 
> The comp.lang.c FAQ has some interesting examples of wildly different
> pointer representations at:
> 
>   http://c-faq.com/null/machexamp.html