Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error

[Dharmik Thakkar <Dharmik.Thakkar@arm.com>]

> On Oct 26, 2018, at 4:05 PM, Wang, Yipeng1 <yipeng1.wang@intel.com> wrote:
> 
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> Sent: Friday, October 26, 2018 1:25 PM
>> To: Dharmik Thakkar <dharmik.thakkar@arm.com>
>> Cc: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org;
>> honnappa.nagarahalli@arm.com; Wang, Yipeng1 <yipeng1.wang@intel.com>
>> Subject: Re: [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error
>> 
>> +Cc Yipeng
>> 
>> 18/09/2018 21:22, Dharmik Thakkar:
>>> Enable print_key_info() function compilation always.
>> 
>> Please see my first comment: you need to show the compilation error
>> in this message. Otherwise, we don't know what you are trying
>> to fix.
>> 
>>> Fixes: af75078fece36 ("first public release")
>>> Cc: stable@dpdk.org
>>> 
>>> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
>>> ---
>>> v2:
>>> * Fix checkpatch coding style issue
>>> * Add "Fixes:" tag
>>> ---
>>> test/test/test_hash.c | 24 +++++++++---------------
>>> 1 file changed, 9 insertions(+), 15 deletions(-)
>>> 
>>> diff --git a/test/test/test_hash.c b/test/test/test_hash.c
>>> index b3db9fd10547..db6442a2b101 100644
>>> --- a/test/test/test_hash.c
>>> +++ b/test/test/test_hash.c
>>> +#define UNIT_TEST_HASH_VERBOSE	0
>>> /*
>>>  * Print out result of unit test hash operation.
>>>  */
>>> -#if defined(UNIT_TEST_HASH_VERBOSE)
>>> static void print_key_info(const char *msg, const struct flow_key *key,
>>> 								int32_t pos)
>>> {
>>> -	uint8_t *p = (uint8_t *)key;
>>> -	unsigned i;
>>> -
>>> -	printf("%s key:0x", msg);
>>> -	for (i = 0; i < sizeof(struct flow_key); i++) {
>>> -		printf("%02X", p[i]);
>>> +	if (UNIT_TEST_HASH_VERBOSE) {
>> 
>> This is very suspicious.
>> Why keeping this code if it is never called?
> 
> [Wang, Yipeng] I assume this is for the convenience for debug. E.g. if the unit test failed,
> developer can set the macro and print more information, but by default the code is not used.
> 
> A quick grep I found  the test_timer_racecond and efd unit test has similar macros. But could anyone
> let me know what is the best coding practice for such purpose in unit test?
Thank you bringing up the discussion, Yipeng. I, too, would like to know the best coding practice for such purposes.

One disadvantage of such macros is: That section of the code is only compiled when the macro is defined. 
For eg., previously, ‘print_key_info()’ did not compile without defining UNIT_TEST_HASH_VERBOSE.
Thus, it’s compilation error(s) are not accounted for always.
> 
> I also wonder what compilation error the original code causes as Thomas indicated.