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

[Thomas Monjalon <thomas@monjalon.net>]

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.