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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.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.
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.

>