commit-slab cleanups
cover commit-slab cleanups
I gathered these while writing an "all merge bases simultaneously" algorithm. Turns out this fancy algorithm loses vs. simply calling get_merge_bases() a lot, so I dropped that. But the cleanups seem valid anyway.
Thomas Rast (2):
commit-slab: document clear_$slabname()
commit-slab: declare functions "static inline"
commit-slab.h | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
Signature · 1 line
1.8.5.rc3.397.g2a3acd5
PATCH 1/2 commit-slab: document clear_$slabname()
The clear_$slabname() function was only documented by source code so far. Write something about it.
--- commit-slab.h | 4 ++++ 1 file changed, 4 insertions(+)
commit-slab.h +4 -0
@@ -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 */
Signature · 1 line
1.8.5.rc3.397.g2a3acd5
Re: [PATCH 1/2] commit-slab: document clear_$slabname()
Quoted 2 lines · Thomas Rast
The clear_$slabname() function was only documented by source code so far. Write something about it.
Good idea.
[...]
commit-slab.h line 30
Quoted 10 lines
commit-slab.h +4
-0@@ -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.
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?
- void clear_indegree(struct indegree *);
Empties the slab. The slab can be reused with the same stride without calling init_indegree again or can be reconfigured to a different stride by calling init_indegree_with_stride.
Call this function before the slab falls out of scope to avoid leaking memory.
Thanks,
Jonathan
Re: [PATCH 1/2] commit-slab: document clear_$slabname()
Quoted 10 lines · Jonathan Nieder
Quoted 4 lines · Thomas Rast
+ *
+ * - voidclear_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).
Quoted 6 lines
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?
[...]
Ok, I see that while I was procrastinating, you sorted this out and Junio merged it to next.
Thanks, both.
Signature · 2 lines
Thomas Rast [email protected]
Re: [PATCH 1/2] commit-slab: document clear_$slabname()
In reply to Thomas RastQuoted 13 lines · Thomas Rast
The clear_$slabname() function was only documented by source code so far. Write something about it.
Signed-off-by Thomas Rast <[email protected]>--- commit-slab.h | 4 ++++ 1 file changed, 4 insertions(+)commit-slab.h +0
-0
commit-slab.h line 25
Quoted 2 lines
commit-slab.h +0
-0@@ -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"?
Quoted 8 lines
commit-slab.h +4
-0* 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 */
Re: [PATCH 1/2] commit-slab: document clear_$slabname()
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Thomas Rast
The clear_$slabname() function was only documented by source code so far. Write something about it.
Signed-off-by Thomas Rast <[email protected]>--- commit-slab.h | 4 ++++ 1 file changed, 4 insertions(+)
Quoted 9 lines
commit-slab.h line 25
Quoted 7 lines
commit-slab.h +0
-0@@ -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"?
And, s/initialied/initialized/
Quoted 12 lines
commit-slab.h line 33
Quoted 8 lines
commit-slab.h +4
-0* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/2 commit-slab: declare functions "static inline"
In reply to Thomas RastThis shuts up compiler warnings about unused functions.
While there, also remove the redundant second declaration of stat_##slabname##realloc.
--- commit-slab.h | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
commit-slab.h +8 -9
@@ -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 */
Signature · 1 line
1.8.5.rc3.397.g2a3acd5
Re: [PATCH 2/2] commit-slab: declare functions "static inline"
Quoted 1 line · Thomas Rast
This shuts up compiler warnings about unused functions.
Thanks.
Quoted 2 lines
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?
Quoted 58 lines
Signed-off-by Thomas Rast <[email protected]>--- commit-slab.h | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)commit-slab.h +8
-9@@ -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 */
Re: [PATCH 2/2] commit-slab: declare functions "static inline"
Quoted 18 lines · Junio C Hamano
Quoted 1 line · Thomas Rast
This shuts up compiler warnings about unused functions.
Thanks.
Quoted 2 lines
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
Quoted 6 lines
slabname ## _at(...) { ... };with a trailing and undesired semicolon?
Oooh. The sudden enlightenment.
Signature · 2 lines
Thomas Rast [email protected]
PATCH v2 1/1 commit-slab: declare functions "static inline"
This shuts up compiler warnings about unused functions. No such warnings are currently triggered, but if someone were to actually use init_NAME_with_stride() as documented, they would get a warning about init_NAME() being unused.
While there, write a comment about why we need two declarations of the same variable.
---
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(-)
commit-slab.h +18 -6
@@ -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 */
Signature · 1 line
1.8.5.rc2.355.g6969a19
Re: [PATCH v2] commit-slab: declare functions "static inline"
Quoted 1 line · Thomas Rast
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.
[...]
commit-slab.h struct slabname { \ · line 104
Quoted 8 lines
commit-slab.h +4
-0@@ -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 ...
Thanks,
Jonathan
Re: [PATCH v2] commit-slab: declare functions "static inline"
Quoted 10 lines · Jonathan Nieder
Quoted 1 line · Thomas Rast
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_ ...
Quoted 6 lines
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.
Signature · 2 lines
Thomas Rast [email protected]
Re: [PATCH v2] commit-slab: declare functions "static inline"
Quoted 18 lines · Thomas Rast
Quoted 10 lines · Jonathan Nieder
Quoted 1 line · Thomas Rast
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_ ...Quoted 6 lines
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.
Quoted 5 lines
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.
The khash functions are very small, so it very well may make sense for them to be inline.
git-compat-util.h (or compat/msvc.h) defines __attribute__(x) to an empty sequence of tokens except on HP C and gcc. Attribute unused has existed at least since GCC 2.95.
Unfortunately HP C doesn't support attribute __unused__. :(
http://h21007.www2.hp.com/portal/download/files/unprot/aCxx/Online_Help/pragmas.htm#Attributes
On the bright side, it would be easy to work around using a conditional definition of MAYBE_UNUSED for the sake of HP C. From August 2010 until March 2011 nobody noticed.
Jonathan
Re: [PATCH v2] commit-slab: declare functions "static inline"
In reply to Jonathan NiederQuoted 32 lines · Jonathan Nieder
Quoted 1 line · Thomas Rast
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.
[...]
commit-slab.h struct slabname { \ · line 104
Quoted 8 lines
commit-slab.h +4
-0@@ -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:
Quoted 2 lines
/* * This seemingly redundant second declaration is required to ...
Hmm, both of these are good points.
Thanks.
Re: [PATCH v2] commit-slab: declare functions "static inline"
In reply to Thomas RastQuoted 6 lines · Thomas Rast
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.
Thanks.
I considered the bottom one the real declaration (with the top one a forward declaration we need to make the result compile), by the way, so there may be no redundancy anywhere ;-)
Quoted 61 lines
commit-slab.h | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)commit-slab.h +18
-6@@ -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 */
Re: [PATCH 2/2] commit-slab: declare functions "static inline"
In reply to Thomas RastThomas,
As you're touching this, perhaps you coud fix this line in slabname_at() too?
s->slab = xrealloc(s->slab, (nth_slab + 1) * sizeof(s->slab));
I think it should be sizeof(*s->slab), not sizeof(s->slab), even though the end result is the same.
Signature · 1 line
Duy
PATCH 1/1 commit-slab: sizeof() the right type in xrealloc
When allocating the slab, the code accidentally computed the array size from s->slab (an elemtype**). The slab is an array of elemtype*, however, so we should take the size of *s->slab.
---
[I hope this comes through clean. git-send-email is currently broken for me, and I'm still investigating, so I have to kludge around it.]
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
Consider the cases mentioned there where void* and char* have different representations from, e.g., int*. Then if elemtype is char, the slab will be too small before this patch.
But I have no idea if any of those are POSIXish.
One interesting, though orthogonal, tidbit is that POSIX actually requires _function_ pointers to have the same representation as void*. From the specification of dlsym(), which depends on this to be able to return function pointers:
RATIONALE
[...] Indeed, the ISO C standard does not require that an object of type void * can hold a pointer to a function. Implementations supporting the XSI extension, however, do require that an object of type void * can hold a pointer to a function.
Thank god for POSIX. So much craziness averted.
[*] Note that C++ method pointers are yet another story. This only applies to the kinds of pointers that C supports.
commit-slab.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
commit-slab.h +1 -1
@@ -91,7 +91,7 @@ struct slabname { \if (s->slab_count <= nth_slab) { \int i; \ s->slab =xrealloc (s->slab, \ - (nth_slab +1 ) *sizeof (s->slab)); \ + (nth_slab +1 ) *sizeof (*s->slab)); \ stat_ ##slabname## realloc++; \ for (i = s->slab_count; i <= nth_slab; i++) \ s->slab[i] = NULL; \
Signature · 1 line
1.8.5.427.g6d3141d
Re: [PATCH] commit-slab: sizeof() the right type in xrealloc
Quoted 3 lines · Thomas Rast
When allocating the slab, the code accidentally computed the array size from
s->slab(an elemtype**). The slab is an array of elemtype*, however, so we should take the size of*s->slab.
Looks obviously correct.
Quoted 9 lines
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.cFAQ has some interesting examples of wildly different pointer representations at:
Note that most of those examples are not different sizes, but rather different null-pointer representations. We are already grossly out of compliance with the standard there, as we typically use memset() to zero-out structs with pointers.
-Peff