DPDK patches and discussions
 help / color / mirror / Atom feed
From: Dharmik Thakkar <Dharmik.Thakkar@arm.com>
To: "Wang, Yipeng1" <yipeng1.wang@intel.com>
Cc: Thomas Monjalon <thomas@monjalon.net>,
	"Richardson, Bruce" <bruce.richardson@intel.com>,
	"De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	nd <nd@arm.com>
Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/hash: solve unit test hash compilation error
Date: Fri, 26 Oct 2018 21:55:36 +0000	[thread overview]
Message-ID: <C17C1B30-23E5-4417-8B66-615FD17D5F05@arm.com> (raw)
In-Reply-To: <D2C4A16CA39F7F4E8E384D204491D7A6614FFF5C@ORSMSX105.amr.corp.intel.com>



> On Oct 26, 2018, at 4:05 PM, Wang, Yipeng1 <yipeng1.wang@intel.com> wrote:
> 
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> Sent: Friday, October 26, 2018 1:25 PM
>> To: Dharmik Thakkar <dharmik.thakkar@arm.com>
>> Cc: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org;
>> honnappa.nagarahalli@arm.com; Wang, Yipeng1 <yipeng1.wang@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: 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.
> 
> I also wonder what compilation error the original code causes as Thomas indicated.


  reply	other threads:[~2018-10-26 21:55 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-27 14:26 [dpdk-dev] [PATCH] " Dharmik Thakkar
2018-08-27 14:41 ` Gavin Hu
2018-09-16  9:59 ` Thomas Monjalon
2018-09-18 19:22 ` [dpdk-dev] [PATCH v2] " Dharmik Thakkar
2018-10-01 20:04   ` Honnappa Nagarahalli
2018-10-26 20:24   ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2018-10-26 21:05     ` Wang, Yipeng1
2018-10-26 21:55       ` Dharmik Thakkar [this message]
2018-10-26 21:59         ` Thomas Monjalon
2018-10-29  4:16           ` Honnappa Nagarahalli
2018-10-29  4:24             ` Thomas Monjalon
2018-10-29  4:54               ` Honnappa Nagarahalli
2018-10-26 21:09     ` Dharmik Thakkar
2018-10-26 21:43   ` [dpdk-dev] [PATCH v3] " Dharmik Thakkar
2018-11-06  2:19     ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=C17C1B30-23E5-4417-8B66-615FD17D5F05@arm.com \
    --to=dharmik.thakkar@arm.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=nd@arm.com \
    --cc=pablo.de.lara.guarch@intel.com \
    --cc=thomas@monjalon.net \
    --cc=yipeng1.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).