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