On Mon, Mar 19, 2018 at 10:40 AM, Jeff Hostetler <git@xxxxxxxxxxxxxxxxx> wrote:
>
>
> On 3/18/2018 4:47 AM, Eric Sunshine wrote:
>>
>> On Sun, Mar 18, 2018 at 4:25 AM, Duy Nguyen <pclouds@xxxxxxxxx> wrote:
>>>
>>> On Sun, Mar 18, 2018 at 3:11 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
>>> wrote:
>>>>
>>>> On Sat, Mar 17, 2018 at 3:53 AM, Nguyễn Thái Ngọc Duy
>>>> <pclouds@xxxxxxxxx> 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.
>
>
> I agree with Eric, this indirection seems unnecessary and confusing.
> Generally, when I see function pointers initialized like that, I
> think that there are plans for additional providers/drivers for that
> functionality, but I don't see that here. And it seems odd.
>
>>
>> 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'.
>>
>
> I see 2 problems.
> 1. The test function in name-hash.c that needs access to private data.
> I'm not a fan of the "_for_testing" suffix, but I'm open. I might
> either leave it as is, or make it a "TEST_" or "test__" prefix (as
> a guide for people used to languages with namespaces.
>
> 2. The new name for cmd_main(). There's no real need why it needs to
> be "test_*", right? Your cmds[] maps the command line string to a
> function, but it could be anything. That is, "cmd_main()" could be
> renamed "cmd__lazy_init_name_hash()". Then you can conceptually
> reserve the "cmd__" prefix throughout t/helper for each test handler.
cmd__ prefix solves my problem nicely. I'll wait for a while before
resending another 30+ patches.
--
Duy
