From mboxrd@z Thu Jan 1 00:00:00 1970 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) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id A9E4421EC3; Mon, 29 Oct 2018 00:24:54 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Mon, 29 Oct 2018 00:24:54 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=WjQFI+m3u8nE8xs7iLWyI04gr4vUjYtzgCK35j55Avk=; b=cwPlbC4jqU2C Ztamvjlj3p68ouWj49RL8BaG4OYZvPSWj+lG77pvDrzPZoD6IkUsKlSXjqqVwj+R y7lDZ7GZahgyuXzSgwZQqp3/ThUKHf5gM8CcNBElaNwz1T8cgh3DNXpL/gkNkepT E5OLHuFDPbcEjcBru3EalBMAVpa1Y7c= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=WjQFI+m3u8nE8xs7iLWyI04gr4vUjYtzgCK35j55A vk=; b=KN2SIWnjbDYf4aZFrb7eTTHX7VF7TAeg9naj0pRjuknGtRpUlorgf5k2G iF0UmRGF6Qkr8bVfjllEDJkZxp3iZShr1uLNm7F0cSSrwQcx3ybZ1xwPQh2SGQYW cELqvtOkhZQvpqYdkFPMC/TsoKpJ0OflN47tYe6cSC5M53B+xGA2DjkDSIhjzSGt 9B+qHJxE133OgoX2r5nwW93cPkpGw6JP+il3Hh+8PyzPDHwu90hXAXTU3JzHNCAs otgp9i/XKAxVZJRUNUAWkFlb03dJtZnn4MZbk7Tx9n7G5ra/QRJQAJnRO2gknRwz tz+83shKZJ61L7U7huoFB+HLVWruA== X-ME-Sender: X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id B5A1BE4430; Mon, 29 Oct 2018 00:24:52 -0400 (EDT) From: Thomas Monjalon To: Honnappa Nagarahalli Cc: Dharmik Thakkar , "Wang, Yipeng1" , "Richardson, Bruce" , "De Lara Guarch, Pablo" , "dev@dpdk.org" , nd Date: Mon, 29 Oct 2018 05:24:59 +0100 Message-ID: <23350481.mpM1qx9Ftk@xps> In-Reply-To: References: <1535379969-19642-1-git-send-email-dharmik.thakkar@arm.com> <4369948.DfioVmEDkk@xps> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Oct 2018 04:24:55 -0000 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.