From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error Date: Mon, 29 Oct 2018 05:24:59 +0100 Message-ID: <23350481.mpM1qx9Ftk@xps> References: <1535379969-19642-1-git-send-email-dharmik.thakkar@arm.com> <4369948.DfioVmEDkk@xps> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cc: Dharmik Thakkar , "Wang, Yipeng1" , "Richardson, Bruce" , "De Lara Guarch, Pablo" , "dev@dpdk.org" , nd To: Honnappa Nagarahalli Return-path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 19AB41D7 for ; Mon, 29 Oct 2018 05:24:55 +0100 (CET) In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > > >>> Signed-off-by: Dharmik Thakkar > > > >>> Reviewed-by: Honnappa Nagarahalli > > > >>> Reviewed-by: Gavin Hu > > > >>> --- > > > >>> 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 =3D (uint8_t *)key; > > > >>> - unsigned i; > > > >>> - > > > >>> - printf("%s key:0x", msg); > > > >>> - for (i =3D 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 k= now 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, =E2=80=98print_key_info()=E2=80=99 did not compi= le without defining > > UNIT_TEST_HASH_VERBOSE. > > > Thus, it=E2=80=99s compilation error(s) are not accounted for always. > >=20 > > 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 prin= ted 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.