From: Jeff Hostetler Date: Mon, 19 Mar 2018 09:40:30 GMT Subject: Re: [PATCH 04/36] t/helper: merge test-lazy-init-name-hash into test-tool Message-ID: In-Reply-To: On 3/18/2018 4:47 AM, Eric Sunshine wrote: > On Sun, Mar 18, 2018 at 4:25 AM, Duy Nguyen wrote: >> On Sun, Mar 18, 2018 at 3:11 AM, Eric Sunshine wrote: >>> On Sat, Mar 17, 2018 at 3:53 AM, Nguyễn Thái Ngọc Duy 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. Just a thought, Thanks, Jeff