On Sun, Mar 18, 2018 at 4:25 AM, Duy Nguyen <pclo...@gmail.com> wrote:
> On Sun, Mar 18, 2018 at 3:11 AM, Eric Sunshine <sunsh...@sunshineco.com>
> wrote:
>> On Sat, Mar 17, 2018 at 3:53 AM, Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
>> wrote:
>>> -extern int test_lazy_init_name_hash(struct index_state *istate, int
>>> try_threaded);
>>> +extern int lazy_init_name_hash_for_testing(struct index_state *istate, int
>>> try_threaded);
>>
>> I get why you renamed this since the "main" function in the test
>> program wants to be called 'test_lazy_init_name_hash'...
>>
>>> diff --git a/t/helper/test-lazy-init-name-hash.c
>>> b/t/helper/test-lazy-init-name-hash.c
>>> @@ -9,6 +10,9 @@ static int perf;
>>> +static int (*init_name_hash)(struct index_state *istate, int try_threaded)
>>> =
>>> + lazy_init_name_hash_for_testing;
>>> +
>>> @@ -33,9 +37,9 @@ static void dump_run(void)
>>> if (single) {
>>> - test_lazy_init_name_hash(&the_index, 0);
>>> + init_name_hash(&the_index, 0);
>>
>> ... but I'm having trouble understanding why this indirection through
>> 'init_name_hash' is used rather than just calling
>> lazy_init_name_hash_for_testing() directly. Am I missing something
>> obvious or is 'init_name_hash' just an unneeded artifact of an earlier
>> iteration before the rename in cache.{c,h}?
>
> Nope. It just feels too long to me and because we're already in the
> test I don't feel the need to repeat _for_testing everywhere. If it's
> confusing, I'll remove init_name_hash.
Without an explanatory in-code comment, I'd guess that someone coming
across this in the future will also stumble over it just like I did in
the review.
What if, instead, you rename test_lazy_init_name_hash() to
lazy_init_name_hash_test(), shifting 'test' from the front to the back
of the name? That way, the name remains the same length at the call
sites in the test helper, and you don't have to introduce the
confusing, otherwise unneeded 'init_name_hash'.
