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

[Thomas Monjalon <thomas@monjalon.net>]

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.