26/10/2018 23:55, Dharmik Thakkar: > > > On Oct 26, 2018, at 4:05 PM, Wang, Yipeng1 <yipeng1.w...@intel.com> wrote: > > > >> -----Original Message----- > >> From: Thomas Monjalon [mailto:tho...@monjalon.net] > >> Sent: Friday, October 26, 2018 1:25 PM > >> To: Dharmik Thakkar <dharmik.thak...@arm.com> > >> Cc: Richardson, Bruce <bruce.richard...@intel.com>; De Lara Guarch, Pablo > >> <pablo.de.lara.gua...@intel.com>; dev@dpdk.org; > >> honnappa.nagaraha...@arm.com; Wang, Yipeng1 <yipeng1.w...@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: sta...@dpdk.org > >>> > >>> Suggested-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com> > >>> Signed-off-by: Dharmik Thakkar <dharmik.thak...@arm.com> > >>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com> > >>> Reviewed-by: Gavin Hu <gavin...@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.
