From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 44D4543BCF; Thu, 7 Mar 2024 14:14:54 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 352A0410D5; Thu, 7 Mar 2024 14:14:54 +0100 (CET) Received: from szxga07-in.huawei.com (szxga07-in.huawei.com [45.249.212.35]) by mails.dpdk.org (Postfix) with ESMTP id 552F54067E for ; Thu, 7 Mar 2024 14:14:52 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.163.44]) by szxga07-in.huawei.com (SkyGuard) with ESMTP id 4Tr8pz64Cpz1QB67; Thu, 7 Mar 2024 21:12:27 +0800 (CST) Received: from dggpeml500024.china.huawei.com (unknown [7.185.36.10]) by mail.maildlp.com (Postfix) with ESMTPS id 4BD97140155; Thu, 7 Mar 2024 21:14:50 +0800 (CST) Received: from [10.67.121.161] (10.67.121.161) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Thu, 7 Mar 2024 21:14:50 +0800 Subject: Re: [PATCH 3/4] argparse: fix argument flags operate as uint32 type To: David Marchand CC: , References: <20240220131502.47510-1-fengchengwen@huawei.com> <20240220131502.47510-4-fengchengwen@huawei.com> From: fengchengwen Message-ID: Date: Thu, 7 Mar 2024 21:14:49 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.161] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To dggpeml500024.china.huawei.com (7.185.36.10) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi David, On 2024/3/7 1:34, David Marchand wrote: > On Tue, Feb 20, 2024 at 2:16 PM Chengwen Feng wrote: >> >> The struct rte_argparse_arg's flags was 64bit type, uint64_t should be >> used instead of uint32_t where the operation happened. > > Something is strange. > An enum in C is represented as an int. > > Plus, this enum type is not used anywhere: > lib/argparse/rte_argparse.h:enum rte_argparse_flag { > lib/argparse/rte_argparse.h: /** @see rte_argparse_flag */ > > I understand the flags are a bitmask. > So please remove this enum and define macros instead. Thanks for point it out, already send v2. > > >> >> Also, the flags' bit16 was also unused, so don't test bit16 in testcase >> test_argparse_invalid_arg_flags. >> >> Fixes: 6c5c6571601c ("argparse: verify argument config") >> Fixes: 31ed9f9f43bb ("argparse: parse parameters") >> >> Signed-off-by: Chengwen Feng >> --- >> app/test/test_argparse.c | 16 ++++++++-------- >> lib/argparse/rte_argparse.c | 4 ++-- >> 2 files changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c >> index c98bcee56d..708a575e16 100644 >> --- a/app/test/test_argparse.c >> +++ b/app/test/test_argparse.c >> @@ -188,7 +188,7 @@ test_argparse_invalid_arg_help(void) >> static int >> test_argparse_invalid_has_val(void) >> { >> - uint32_t set_mask[] = { 0, >> + uint64_t set_mask[] = { 0, >> RTE_ARGPARSE_ARG_NO_VALUE, >> RTE_ARGPARSE_ARG_OPTIONAL_VALUE >> }; >> @@ -197,7 +197,7 @@ test_argparse_invalid_has_val(void) >> int ret; >> >> obj = test_argparse_init_obj(); >> - obj->args[0].flags &= ~0x3u; >> + obj->args[0].flags &= ~0x3ull; > > If flags is a uint64_t, use RTE_BIT64(). > > > I don't know the argparse API, but why do we need this hardcoded (and > hard to understand) ~3 value? The lowest two bits was a represent whether has value. > Can it be expressed with the flags defined in the API? Its not part of API, I defined a macro in test_argparse.c to express its meaning in v2. > > >> ret = rte_argparse_parse(obj, default_argc, default_argv); >> TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!"); >> >> @@ -205,7 +205,7 @@ test_argparse_invalid_has_val(void) >> obj = test_argparse_init_obj(); >> obj->args[0].name_long = "abc"; >> obj->args[0].name_short = NULL; >> - obj->args[0].flags &= ~0x3u; >> + obj->args[0].flags &= ~0x3ull; >> obj->args[0].flags |= set_mask[index]; >> ret = rte_argparse_parse(obj, default_argc, default_argv); >> TEST_ASSERT(ret == -EINVAL, "Argparse parse expect failed!"); >> @@ -269,7 +269,7 @@ test_argparse_invalid_arg_flags(void) >> int ret; >> >> obj = test_argparse_init_obj(); >> - obj->args[0].flags |= ~0x107FFu; >> + obj->args[0].flags |= ~0x7FFull; > > Same comments as above. Done in v2, defined macros in test_argpase.c and express with multi-marco's or value. Thanks > >