From: Derrick Stolee <stolee@gmail.com>
Date: Fri, 21 Sep 2018 17:41:35 GMT
Subject: Re: [PATCH 9/9] commit-reach.h: add missing declarations (hdr-check)
Message-ID: <15ffe24b-0ebb-6723-07af-703bc45becba@gmail.com>
In-Reply-To: <2809251f-6eba-b0ac-68fe-b972809ccff7@ramsayjones.plus.com>

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