Re: [PATCH v5 02/12] sha1-file: provide functions to look up hash algorithms

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>,
	"Derrick Stolee" <stolee@gmail.com>,
	git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Duy Nguyen" <pclouds@gmail.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Jakub Narebski" <jnareb@gmail.com>,
	"Christian Couder" <christian.couder@gmail.com>
Subject: Re: [PATCH v5 02/12] sha1-file: provide functions to look up hash algorithms
Date: Wed, 14 Nov 2018 03:53:20 +0000	[thread overview]
Message-ID: <79b436d4-48f9-8e38-0dbe-1831aa3fc208@ramsayjones.plus.com> (raw)
In-Reply-To: <20181114021118.GN890086@genre.crustytoothpaste.net>

On 14/11/2018 02:11, brian m. carlson wrote:
> On Wed, Nov 14, 2018 at 12:11:07AM +0000, Ramsay Jones wrote:
>>
>>
>> On 13/11/2018 18:42, Derrick Stolee wrote:
>>> On 11/4/2018 6:44 PM, brian m. carlson wrote:
>>>> +int hash_algo_by_name(const char *name)
>>>> +{
>>>> +    int i;
>>>> +    if (!name)
>>>> +        return GIT_HASH_UNKNOWN;
>>>> +    for (i = 1; i < GIT_HASH_NALGOS; i++)
>>>> +        if (!strcmp(name, hash_algos[i].name))
>>>> +            return i;
>>>> +    return GIT_HASH_UNKNOWN;
>>>> +}
>>>> +
>>>
>>> Today's test coverage report [1] shows this method is not covered in the test suite. Looking at 'pu', it doesn't have any callers.
>>>
>>> Do you have a work in progress series that will use this? Could we add a test-tool to exercise this somehow?
>>
>> There are actually 4 unused external symbols resulting from Brian's
>> 'bc/sha-256' branch. The new unused externals in 'pu' looks like:
>>
>>     $ diff nsc psc
>>     37a38,39
>>     > hex.o	- hash_to_hex
> 
> I have code that uses this in my object-id-part15 series.  I also have
> another series coming after this one that makes heavy use of it.
> 
>>     > hex.o	- hash_to_hex_algop_r
> 
> I believe this is because it's inline, since it is indeed used just a
> few lines below its definition.  I'll drop the inline, since it's meant
> to be externally visible.

No, this has nothing to do with the 'inline', it is simply not
called outside of hex.c (at present). If you look at the assembler
(objdump -d hex.o), you will find practically all of the function
calls in that file are inlined (even those not marked with 'inline').

[I think the external declaration in cache.h forces the compiler to
add the external definition, despite the 'inline'. If you remove the
'inline' and re-compile and disassemble again, the result is identical.]

Thanks for confirming upcoming patches will add uses for all of
these functions - I suspected that would be the case.

Thanks!

ATB,
Ramsay Jones

> 
>>     > sha1-file.o	- hash_algo_by_id
> 
> This will be used when I write pack index v3, which will be in my
> object-id-part15 series.
> 
>>     > sha1-file.o	- hash_algo_by_name
> 
> This is used in my object-id-part15 series.
> 