Re: [PATCH 04/36] t/helper: merge test-lazy-init-name-hash into test-tool
On Mon, Mar 19, 2018 at 10:40 AM, Jeff Hostetler wrote:
>
>
> 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.
cmd__ prefix solves my problem nicely. I'll wait for a while before
resending another 30+ patches.
--
Duy
Re: [PATCH 04/36] t/helper: merge test-lazy-init-name-hash into test-tool
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
Re: [PATCH 04/36] t/helper: merge test-lazy-init-name-hash into test-tool
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.
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'.
Re: [PATCH 04/36] t/helper: merge test-lazy-init-name-hash into test-tool
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:
>> diff --git a/cache.h b/cache.h
>> @@ -333,7 +333,7 @@ struct index_state {
>> -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.
--
Duy
Re: [PATCH 04/36] t/helper: merge test-lazy-init-name-hash into test-tool
On Sat, Mar 17, 2018 at 3:53 AM, Nguyễn Thái Ngọc Duy wrote:
> diff --git a/cache.h b/cache.h
> @@ -333,7 +333,7 @@ struct index_state {
> -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}?
