{"thread":{"id":"49376","subject":"[PATCH 9/9] commit-reach.h: add missing declarations (hdr-check)","startedAt":"2018-09-19T00:15:32Z","lastAt":"2018-09-21T17:41:39Z","messageCount":4,"participants":["Ramsay Jones","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"358433","messageId":"089b2111-b7e9-6795-b04a-ed259f78796a@ramsayjones.plus.com","threadId":"49376","inReplyTo":null,"subject":"[PATCH 9/9] commit-reach.h: add missing declarations (hdr-check)","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-09-19T00:15:27Z","receivedAt":"2018-09-19T00:15:32Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n commit-reach.h | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-reach.h b/commit-reach.h\nindex 7d313e2975..f41d8f6ba3 100644\n--- a/commit-reach.h\n+++ b/commit-reach.h\n@@ -1,12 +1,13 @@\n #ifndef __COMMIT_REACH_H__\n #define __COMMIT_REACH_H__\n \n+#include \"commit.h\"\n #include \"commit-slab.h\"\n \n-struct commit;\n struct commit_list;\n-struct contains_cache;\n struct ref_filter;\n+struct object_id;\n+struct object_array;\n \n struct commit_list *get_merge_bases_many(struct commit *one,\n \t\t\t\t\t int n,\n-- \n2.19.0\n"},{"id":"358506","messageId":"eb77d563-8b3e-9b00-59b8-b050e6378c69@gmail.com","threadId":"49376","inReplyTo":"089b2111-b7e9-6795-b04a-ed259f78796a@ramsayjones.plus.com","subject":"Re: [PATCH 9/9] commit-reach.h: add missing declarations (hdr-check)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-09-19T23:38:57Z","receivedAt":"2018-09-20T12:51:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/18/2018 8:15 PM, Ramsay Jones wrote:\n> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n> ---\n>   commit-reach.h | 5 +++--\n>   1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/commit-reach.h b/commit-reach.h\n> index 7d313e2975..f41d8f6ba3 100644\n> --- a/commit-reach.h\n> +++ b/commit-reach.h\n> @@ -1,12 +1,13 @@\n>   #ifndef __COMMIT_REACH_H__\n>   #define __COMMIT_REACH_H__\n>   \n> +#include \"commit.h\"\n>   #include \"commit-slab.h\"\n>   \n> -struct commit;\n>   struct commit_list;\n> -struct contains_cache;\n\nInteresting that you needed all of commit.h here, and these 'struct \ncommit' and 'struct contains_cache' were not enough. Was there a reason \nyou needed the entire header file instead of just adding a missing \nstruct declaration?\n\n>   struct ref_filter;\n> +struct object_id;\n> +struct object_array;\n>   \n>   struct commit_list *get_merge_bases_many(struct commit *one,\n>   \t\t\t\t\t int n,\n\nThese look like standard inclusions. Thanks!\n"},{"id":"358514","messageId":"2809251f-6eba-b0ac-68fe-b972809ccff7@ramsayjones.plus.com","threadId":"49376","inReplyTo":"eb77d563-8b3e-9b00-59b8-b050e6378c69@gmail.com","subject":"Re: [PATCH 9/9] commit-reach.h: add missing declarations (hdr-check)","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-09-20T15:35:16Z","receivedAt":"2018-09-20T15:35:20Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 20/09/18 00:38, Derrick Stolee wrote:\n> On 9/18/2018 8:15 PM, Ramsay Jones wrote:\n>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n>> ---\n>>   commit-reach.h | 5 +++--\n>>   1 file changed, 3 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/commit-reach.h b/commit-reach.h\n>> index 7d313e2975..f41d8f6ba3 100644\n>> --- a/commit-reach.h\n>> +++ b/commit-reach.h\n>> @@ -1,12 +1,13 @@\n>>   #ifndef __COMMIT_REACH_H__\n>>   #define __COMMIT_REACH_H__\n>>   +#include \"commit.h\"\n>>   #include \"commit-slab.h\"\n>>   -struct commit;\n>>   struct commit_list;\n>> -struct contains_cache;\n> \n> 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?\n\nActually, the original version of this patch didn't add that\ninclude! I changed my mind just before sending this series\nout, since I felt the original patch was conflating two issues.\n\nThe reason for the #include of 'commit.h' in this patch, but\nnot with my original patch, has to to with the inline functions\nused in the commit-slab implementation. My original patch used\nthe 'commit-slab-{decl,impl}.h' header files to sidestep the\nneed for the definition of 'struct commit'.\n\nI have included an 'RFC on-top' patch below to show you what I\nhad in mind. NOTE: I had not finished writing the commit message\nand you could argue that the 'implement' macro should go in the\nref-filter.c file, since that is were the contains_cache is \n'defined and initialised'. I had not intended to send this to\nthe list ... :-D\n\nNote that, if you compile with optimizations disabled, then\nrun this script:\n\n  $ cat -n dup-static.sh\n       1 #!/bin/sh\n       2 \n       3 nm $1 | grep ' t ' | cut -d' ' -f3 | sort | uniq -c |\n       4 \tsort -rn | grep -v '      1'\n  $ \n\n  $ ./dup-static.sh git | grep contains\n       24 init_contains_cache_with_stride\n       24 init_contains_cache\n       24 contains_cache_peek\n       24 contains_cache_at_peek\n       24 contains_cache_at\n       24 clear_contains_cache\n  $ \n  \nyou will find 24 copies of the commit-slab routines for the\ncontains_cache. Of course, when you enable optimizations again,\nthese duplicate static functions (mostly) disappear. (Two static\nfunctions remain, the rest are actually inlined at -O2).\n\nHowever, if you apply the patch below, you end up with all of\nthe functions in the contains_cache commit-slab implementation\nas external functions. Some of those functions are never called,\nso it's not necessarily a win ... ;-)\n\nATB,\nRamsay Jones\n\n-- >8 --\nSubject: [PATCH] commit-reach: use a shared commit-slab for the contains_cache\n\nCommit a9f1f1f9f8 (\"commit-slab.h: code split\", 2018-05-19) separated\nthe commit-slab interface from its implementation, to allow for the\ndefinition of a public commit-slab data structure. This enabled us to\navoid including the commit-slab implementation in a header file, which\ncould result in the replication of the commit-slab functions in each\ncompilation unit in which it was included.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n commit-reach.c | 3 +++\n commit-reach.h | 6 +++---\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex 622eeb313d..7223cacdd1 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"commit.h\"\n+#include \"commit-slab-impl.h\"\n #include \"commit-graph.h\"\n #include \"decorate.h\"\n #include \"prio-queue.h\"\n@@ -18,6 +19,8 @@\n \n static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT);\n \n+implement_shared_commit_slab(contains_cache, enum contains_result);\n+\n static int queue_has_nonstale(struct prio_queue *queue)\n {\n \tint i;\ndiff --git a/commit-reach.h b/commit-reach.h\nindex f41d8f6ba3..acf3b2d188 100644\n--- a/commit-reach.h\n+++ b/commit-reach.h\n@@ -1,9 +1,9 @@\n #ifndef __COMMIT_REACH_H__\n #define __COMMIT_REACH_H__\n \n-#include \"commit.h\"\n-#include \"commit-slab.h\"\n+#include \"commit-slab-decl.h\"\n \n+struct commit;\n struct commit_list;\n struct ref_filter;\n struct object_id;\n@@ -55,7 +55,7 @@ enum contains_result {\n \tCONTAINS_YES\n };\n \n-define_commit_slab(contains_cache, enum contains_result);\n+define_shared_commit_slab(contains_cache, enum contains_result);\n \n int commit_contains(struct ref_filter *filter, struct commit *commit,\n \t\t    struct commit_list *list, struct contains_cache *cache);\n-- \n2.19.0\n\n"},{"id":"358629","messageId":"15ffe24b-0ebb-6723-07af-703bc45becba@gmail.com","threadId":"49376","inReplyTo":"2809251f-6eba-b0ac-68fe-b972809ccff7@ramsayjones.plus.com","subject":"Re: [PATCH 9/9] commit-reach.h: add missing declarations (hdr-check)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-09-21T17:41:35Z","receivedAt":"2018-09-21T17:41:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/20/2018 11:35 AM, Ramsay Jones wrote:\n>\n> On 20/09/18 00:38, Derrick Stolee wrote:\n>> On 9/18/2018 8:15 PM, Ramsay Jones wrote:\n>>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n>>> ---\n>>>    commit-reach.h | 5 +++--\n>>>    1 file changed, 3 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/commit-reach.h b/commit-reach.h\n>>> index 7d313e2975..f41d8f6ba3 100644\n>>> --- a/commit-reach.h\n>>> +++ b/commit-reach.h\n>>> @@ -1,12 +1,13 @@\n>>>    #ifndef __COMMIT_REACH_H__\n>>>    #define __COMMIT_REACH_H__\n>>>    +#include \"commit.h\"\n>>>    #include \"commit-slab.h\"\n>>>    -struct commit;\n>>>    struct commit_list;\n>>> -struct contains_cache;\n>> 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?\n> Actually, the original version of this patch didn't add that\n> include! I changed my mind just before sending this series\n> out, since I felt the original patch was conflating two issues.\n>\n> The reason for the #include of 'commit.h' in this patch, but\n> not with my original patch, has to to with the inline functions\n> used in the commit-slab implementation. My original patch used\n> the 'commit-slab-{decl,impl}.h' header files to sidestep the\n> need for the definition of 'struct commit'.\n>\n> I have included an 'RFC on-top' patch below to show you what I\n> had in mind. NOTE: I had not finished writing the commit message\n> and you could argue that the 'implement' macro should go in the\n> ref-filter.c file, since that is were the contains_cache is\n> 'defined and initialised'. I had not intended to send this to\n> the list ... :-D\n>\n> Note that, if you compile with optimizations disabled, then\n> run this script:\n>\n>    $ cat -n dup-static.sh\n>         1 #!/bin/sh\n>         2\n>         3 nm $1 | grep ' t ' | cut -d' ' -f3 | sort | uniq -c |\n>         4 \tsort -rn | grep -v '      1'\n>    $\n>\n>    $ ./dup-static.sh git | grep contains\n>         24 init_contains_cache_with_stride\n>         24 init_contains_cache\n>         24 contains_cache_peek\n>         24 contains_cache_at_peek\n>         24 contains_cache_at\n>         24 clear_contains_cache\n>    $\n>    \n> you will find 24 copies of the commit-slab routines for the\n> contains_cache. Of course, when you enable optimizations again,\n> these duplicate static functions (mostly) disappear. (Two static\n> functions remain, the rest are actually inlined at -O2).\n>\n> However, if you apply the patch below, you end up with all of\n> the functions in the contains_cache commit-slab implementation\n> as external functions. Some of those functions are never called,\n> so it's not necessarily a win ... ;-)\n\nThanks for the explanation! I prefer your #include \"commit.h\" above to \nthe alternative.\n\nThanks,\n\n-Stolee\n\n"}]}