On Mon, Mar 19, 2018 at 10:40 AM, Jeff Hostetler <g...@jeffhostetler.com> wrote: > > > On 3/18/2018 4:47 AM, Eric Sunshine wrote: >> >> 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. > > > 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