[v2] test/hash: solve unit test hash compilation error

  Enable print_key_info() function compilation always.

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(-)
  

> 
> Enable print_key_info() function compilation always.
> 
> 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
> @@ -80,29 +80,23 @@ static uint32_t pseudo_hash(__attribute__((unused))
> const void *keys,
>  	return 3;
>  }
> 
> +#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) {
> +		const uint8_t *p = (const uint8_t *)key;
> +		unsigned int i;
> +
> +		printf("%s key:0x", msg);
> +		for (i = 0; i < sizeof(struct flow_key); i++)
> +			printf("%02X", p[i]);
> +		printf(" @ pos %d\n", pos);
>  	}
> -	printf(" @ pos %d\n", pos);
> -}
> -#else
> -static void print_key_info(__attribute__((unused)) const char *msg,
> -		__attribute__((unused)) const struct flow_key *key,
> -		__attribute__((unused)) int32_t pos)
> -{
>  }
> -#endif
> 
>  /* Keys used by unit test functions */
>  static struct flow_key keys[5] = { {
> --
> 2.7.4
Acked-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
  

+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?

> +		const uint8_t *p = (const uint8_t *)key;
> +		unsigned int i;
> +
> +		printf("%s key:0x", msg);
> +		for (i = 0; i < sizeof(struct flow_key); i++)
> +			printf("%02X", p[i]);
> +		printf(" @ pos %d\n", pos);
>  	}
> -	printf(" @ pos %d\n", pos);
> -}
> -#else
> -static void print_key_info(__attribute__((unused)) const char *msg,
> -		__attribute__((unused)) const struct flow_key *key,
> -		__attribute__((unused)) int32_t pos)
> -{
>  }
> -#endif
  

>-----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?

I also wonder what compilation error the original code causes as Thomas indicated.
  

> On Oct 26, 2018, at 3:24 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> +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.
Sure, I will submit a new version with the compilation error message.
> 
>> 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?
The function ‘print_key_info’ is being called at different places and developer can set the macro, to print debug information.
> 
>> +		const uint8_t *p = (const uint8_t *)key;
>> +		unsigned int i;
>> +
>> +		printf("%s key:0x", msg);
>> +		for (i = 0; i < sizeof(struct flow_key); i++)
>> +			printf("%02X", p[i]);
>> +		printf(" @ pos %d\n", pos);
>> 	}
>> -	printf(" @ pos %d\n", pos);
>> -}
>> -#else
>> -static void print_key_info(__attribute__((unused)) const char *msg,
>> -		__attribute__((unused)) const struct flow_key *key,
>> -		__attribute__((unused)) int32_t pos)
>> -{
>> }
>> -#endif
  

> 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.
  

26/10/2018 23:55, Dharmik Thakkar:
> 
> > 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.

The compilation time options are generally bad.
In this case, we could use the log level as a condition for printing.
  

> > >> 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.
> 
> The compilation time options are generally bad.
> In this case, we could use the log level as a condition for printing.
This is test code. So, printing the extra log under a separate flag like UNIT_TEST_HASH_VERBOSE is ok.
When I was debugging the code, I enabled it and it did not compile. This patch ensures that the code is compiled always. But the extra logs are printed only when UNIT_TEST_HASH_VERBOSE is set to non-zero.

>
  

29/10/2018 05:16, Honnappa Nagarahalli:
> > > >> 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.
> > 
> > The compilation time options are generally bad.
> > In this case, we could use the log level as a condition for printing.
> This is test code. So, printing the extra log under a separate flag like UNIT_TEST_HASH_VERBOSE is ok.
> When I was debugging the code, I enabled it and it did not compile. This patch ensures that the code is compiled always. But the extra logs are printed only when UNIT_TEST_HASH_VERBOSE is set to non-zero.

If you keep a compilation time flag, there is a big chance that it becomes
buggy again.
  

> 
> 29/10/2018 05:16, Honnappa Nagarahalli:
> > > > >> 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.
> > >
> > > The compilation time options are generally bad.
> > > In this case, we could use the log level as a condition for printing.
> > This is test code. So, printing the extra log under a separate flag like
> UNIT_TEST_HASH_VERBOSE is ok.
> > When I was debugging the code, I enabled it and it did not compile. This
> patch ensures that the code is compiled always. But the extra logs are printed
> only when UNIT_TEST_HASH_VERBOSE is set to non-zero.
> 
> If you keep a compilation time flag, there is a big chance that it becomes
> buggy again.
> 
I believe you meant, buggy during run time. If yes, I agree. This patch removes only compile time bugs.
Looks like using 'rte_log_set_level', one can enable the logs only for hash library. So, it can be used for unit tests as well rather than defining another flag.
However, it will not solve the run time bugs as the log level will be set to not print the info by default. IMO, it does not make sense to print this all the time.