| Message ID | 089b2111-b7e9-6795-b04a-ed259f78796a@ramsayjones.plus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] Makefile: add a hdr-check target | expand |
On 9/18/2018 8:15 PM, Ramsay Jones wrote: > Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> > --- > commit-reach.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/commit-reach.h b/commit-reach.h > index 7d313e2975..f41d8f6ba3 100644 > --- a/commit-reach.h > +++ b/commit-reach.h > @@ -1,12 +1,13 @@ > #ifndef __COMMIT_REACH_H__ > #define __COMMIT_REACH_H__ > > +#include "commit.h" > #include "commit-slab.h" > > -struct commit; > struct commit_list; > -struct contains_cache; Interesting that you needed all of commit.h here, and these 'struct commit' and 'struct contains_cache' were not enough. Was there a reason you needed the entire header file instead of just adding a missing struct declaration? > struct ref_filter; > +struct object_id; > +struct object_array; > > struct commit_list *get_merge_bases_many(struct commit *one, > int n, These look like standard inclusions. Thanks!
On 20/09/18 00:38, Derrick Stolee wrote: > On 9/18/2018 8:15 PM, Ramsay Jones wrote: >> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> >> --- >> commit-reach.h | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/commit-reach.h b/commit-reach.h >> index 7d313e2975..f41d8f6ba3 100644 >> --- a/commit-reach.h >> +++ b/commit-reach.h >> @@ -1,12 +1,13 @@ >> #ifndef __COMMIT_REACH_H__ >> #define __COMMIT_REACH_H__ >> +#include "commit.h" >> #include "commit-slab.h" >> -struct commit; >> struct commit_list; >> -struct contains_cache; > > Interesting that you needed all of commit.h here, and these 'struct commit' and 'struct contains_cache' were not enough. Was there a reason you needed the entire header file instead of just adding a missing struct declaration? Actually, the original version of this patch didn't add that include! I changed my mind just before sending this series out, since I felt the original patch was conflating two issues. The reason for the #include of 'commit.h' in this patch, but not with my original patch, has to to with the inline functions used in the commit-slab implementation. My original patch used the 'commit-slab-{decl,impl}.h' header files to sidestep the need for the definition of 'struct commit'. I have included an 'RFC on-top' patch below to show you what I had in mind. NOTE: I had not finished writing the commit message and you could argue that the 'implement' macro should go in the ref-filter.c file, since that is were the contains_cache is 'defined and initialised'. I had not intended to send this to the list ... :-D Note that, if you compile with optimizations disabled, then run this script: $ cat -n dup-static.sh 1 #!/bin/sh 2 3 nm $1 | grep ' t ' | cut -d' ' -f3 | sort | uniq -c | 4 sort -rn | grep -v ' 1' $ $ ./dup-static.sh git | grep contains 24 init_contains_cache_with_stride 24 init_contains_cache 24 contains_cache_peek 24 contains_cache_at_peek 24 contains_cache_at 24 clear_contains_cache $ you will find 24 copies of the commit-slab routines for the contains_cache. Of course, when you enable optimizations again, these duplicate static functions (mostly) disappear. (Two static functions remain, the rest are actually inlined at -O2). However, if you apply the patch below, you end up with all of the functions in the contains_cache commit-slab implementation as external functions. Some of those functions are never called, so it's not necessarily a win ... ;-) ATB, Ramsay Jones -- >8 -- Subject: [PATCH] commit-reach: use a shared commit-slab for the contains_cache Commit a9f1f1f9f8 ("commit-slab.h: code split", 2018-05-19) separated the commit-slab interface from its implementation, to allow for the definition of a public commit-slab data structure. This enabled us to avoid including the commit-slab implementation in a header file, which could result in the replication of the commit-slab functions in each compilation unit in which it was included. Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- commit-reach.c | 3 +++ commit-reach.h | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/commit-reach.c b/commit-reach.c index 622eeb313d..7223cacdd1 100644 --- a/commit-reach.c +++ b/commit-reach.c @@ -1,5 +1,6 @@ #include "cache.h" #include "commit.h" +#include "commit-slab-impl.h" #include "commit-graph.h" #include "decorate.h" #include "prio-queue.h" @@ -18,6 +19,8 @@ static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT); +implement_shared_commit_slab(contains_cache, enum contains_result); + static int queue_has_nonstale(struct prio_queue *queue) { int i; diff --git a/commit-reach.h b/commit-reach.h index f41d8f6ba3..acf3b2d188 100644 --- a/commit-reach.h +++ b/commit-reach.h @@ -1,9 +1,9 @@ #ifndef __COMMIT_REACH_H__ #define __COMMIT_REACH_H__ -#include "commit.h" -#include "commit-slab.h" +#include "commit-slab-decl.h" +struct commit; struct commit_list; struct ref_filter; struct object_id; @@ -55,7 +55,7 @@ enum contains_result { CONTAINS_YES }; -define_commit_slab(contains_cache, enum contains_result); +define_shared_commit_slab(contains_cache, enum contains_result); int commit_contains(struct ref_filter *filter, struct commit *commit, struct commit_list *list, struct contains_cache *cache);
On 9/20/2018 11:35 AM, Ramsay Jones wrote: > > On 20/09/18 00:38, Derrick Stolee wrote: >> On 9/18/2018 8:15 PM, Ramsay Jones wrote: >>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> >>> --- >>> commit-reach.h | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/commit-reach.h b/commit-reach.h >>> index 7d313e2975..f41d8f6ba3 100644 >>> --- a/commit-reach.h >>> +++ b/commit-reach.h >>> @@ -1,12 +1,13 @@ >>> #ifndef __COMMIT_REACH_H__ >>> #define __COMMIT_REACH_H__ >>> +#include "commit.h" >>> #include "commit-slab.h" >>> -struct commit; >>> struct commit_list; >>> -struct contains_cache; >> Interesting that you needed all of commit.h here, and these 'struct commit' and 'struct contains_cache' were not enough. Was there a reason you needed the entire header file instead of just adding a missing struct declaration? > Actually, the original version of this patch didn't add that > include! I changed my mind just before sending this series > out, since I felt the original patch was conflating two issues. > > The reason for the #include of 'commit.h' in this patch, but > not with my original patch, has to to with the inline functions > used in the commit-slab implementation. My original patch used > the 'commit-slab-{decl,impl}.h' header files to sidestep the > need for the definition of 'struct commit'. > > I have included an 'RFC on-top' patch below to show you what I > had in mind. NOTE: I had not finished writing the commit message > and you could argue that the 'implement' macro should go in the > ref-filter.c file, since that is were the contains_cache is > 'defined and initialised'. I had not intended to send this to > the list ... :-D > > Note that, if you compile with optimizations disabled, then > run this script: > > $ cat -n dup-static.sh > 1 #!/bin/sh > 2 > 3 nm $1 | grep ' t ' | cut -d' ' -f3 | sort | uniq -c | > 4 sort -rn | grep -v ' 1' > $ > > $ ./dup-static.sh git | grep contains > 24 init_contains_cache_with_stride > 24 init_contains_cache > 24 contains_cache_peek > 24 contains_cache_at_peek > 24 contains_cache_at > 24 clear_contains_cache > $ > > you will find 24 copies of the commit-slab routines for the > contains_cache. Of course, when you enable optimizations again, > these duplicate static functions (mostly) disappear. (Two static > functions remain, the rest are actually inlined at -O2). > > However, if you apply the patch below, you end up with all of > the functions in the contains_cache commit-slab implementation > as external functions. Some of those functions are never called, > so it's not necessarily a win ... ;-) Thanks for the explanation! I prefer your #include "commit.h" above to the alternative. Thanks, -Stolee
diff --git a/commit-reach.h b/commit-reach.h index 7d313e2975..f41d8f6ba3 100644 --- a/commit-reach.h +++ b/commit-reach.h @@ -1,12 +1,13 @@ #ifndef __COMMIT_REACH_H__ #define __COMMIT_REACH_H__ +#include "commit.h" #include "commit-slab.h" -struct commit; struct commit_list; -struct contains_cache; struct ref_filter; +struct object_id; +struct object_array; struct commit_list *get_merge_bases_many(struct commit *one, int n,
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- commit-reach.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)