From: David Marchand <david.marchand@redhat.com>
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: dev@dpdk.org, roretzla@linux.microsoft.com,
Sunil Kumar Kori <skori@marvell.com>,
Rakesh Kudurumalla <rkudurumalla@marvell.com>,
Jerin Jacob <jerinj@marvell.com>,
Srikanth Yalavarthi <syalavarthi@marvell.com>,
Cristian Dumitrescu <cristian.dumitrescu@intel.com>,
Aman Singh <aman.deep.singh@intel.com>,
Yuying Zhang <yuying.zhang@intel.com>,
Brian Dooley <brian.dooley@intel.com>,
Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
Subject: Re: [PATCH] eal: add helper to skip whitespaces
Date: Thu, 15 Feb 2024 10:08:40 +0100 [thread overview]
Message-ID: <CAJFAV8yX6FR5UvWQZ5iSgCW24Sja2fP7vmcjbxTUPEwHL09rCA@mail.gmail.com> (raw)
In-Reply-To: <Zcy6-HbtdyjajELS@bricha3-mobl1.ger.corp.intel.com>
On Wed, Feb 14, 2024 at 2:07 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Feb 14, 2024 at 01:12:19PM +0100, David Marchand wrote:
> > Reduce code duplication by providing a simple inline helper.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > app/graph/utils.c | 13 ++------
> > app/test-eventdev/parser.c | 14 ++++-----
> > app/test-eventdev/parser.h | 8 -----
> > app/test-mldev/parser.c | 17 ++++++-----
> > app/test-mldev/parser.h | 8 -----
> > app/test-pmd/cmdline_tm.c | 13 ++------
> > app/test/test_string_fns.c | 35 ++++++++++++++++++++++
> > examples/fips_validation/fips_validation.c | 16 +++-------
> > examples/ip_pipeline/parser.c | 14 ++++-----
> > examples/ip_pipeline/parser.h | 8 -----
> > examples/pipeline/cli.c | 13 ++------
> > lib/eal/include/rte_string_fns.h | 27 +++++++++++++++++
> > 12 files changed, 98 insertions(+), 88 deletions(-)
> >
> > diff --git a/app/graph/utils.c b/app/graph/utils.c
> > index c7b6ae83cf..1f5ee68273 100644
> > --- a/app/graph/utils.c
> > +++ b/app/graph/utils.c
> > @@ -9,17 +9,10 @@
> > #include <string.h>
> >
> > #include <rte_common.h>
> > +#include <rte_string_fns.h>
> >
> > #include "module_api.h"
> >
> > -#define white_spaces_skip(pos) \
> > -({ \
> > - __typeof__(pos) _p = (pos); \
> > - for ( ; isspace(*_p); _p++) \
> > - ; \
> > - _p; \
> > -})
> > -
> > static void
> > hex_string_to_uint64(uint64_t *dst, const char *hexs)
> > {
> > @@ -47,7 +40,7 @@ parser_uint64_read(uint64_t *value, const char *p)
> > char *next;
> > uint64_t val;
> >
> > - p = white_spaces_skip(p);
> > + p = rte_str_skip_whitespaces(p);
> > if (!isdigit(*p))
> > return -EINVAL;
> >
> > @@ -73,7 +66,7 @@ parser_uint64_read(uint64_t *value, const char *p)
> > break;
> > }
> >
> > - p = white_spaces_skip(p);
> > + p = rte_str_skip_whitespaces(p);
>
> I like the idea of this. However, a question on naming. Do we want to make
> it clear in the name that it only skips leading whitespace, and doesn't
> e.g. trim whitespace off the end? How about:
>
> rte_str_skip_leading_spaces()
>
> It's longer, but I wonder if it's a bit clearer.
It is clearer.
>
> Also, might it be worthwhile providing an "rte_str_ltrim()" function along
> with this, basically combining the skip_spaces call, with an memmove? Most
> string handling in DPDK is not perf-sensitive and so a function with
> similar name and behaviour to the python ltrim function may be appreciated
> for its simplicity.
Well, most intree users won't touch the data (and simply pass around
const char * pointers).
This new API you propose is probably fine, but I suspect it won't be
of much use for now.
Looking at those intree users, the stripping of whitespaces is just
the tip of the iceberg.
Every users is parsing integers etc...
So maybe a better work would be to list what is needed and provide such helpers.
This would further reduce code duplication.
To be franck, I have no big motivation behind this patch.
I found out this copy/paste statement expression macro everywhere
while reviewing Tyler series, and I wanted to shoot it.
If someone wants to take over, they are welcome.
--
David Marchand
next prev parent reply other threads:[~2024-02-15 9:08 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-14 12:12 David Marchand
2024-02-14 13:07 ` Bruce Richardson
2024-02-15 9:08 ` David Marchand [this message]
2024-02-15 9:10 ` Bruce Richardson
2024-02-15 17:32 ` Tyler Retzlaff
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=CAJFAV8yX6FR5UvWQZ5iSgCW24Sja2fP7vmcjbxTUPEwHL09rCA@mail.gmail.com \
--to=david.marchand@redhat.com \
--cc=aman.deep.singh@intel.com \
--cc=brian.dooley@intel.com \
--cc=bruce.richardson@intel.com \
--cc=cristian.dumitrescu@intel.com \
--cc=dev@dpdk.org \
--cc=gmuthukrishn@marvell.com \
--cc=jerinj@marvell.com \
--cc=rkudurumalla@marvell.com \
--cc=roretzla@linux.microsoft.com \
--cc=skori@marvell.com \
--cc=syalavarthi@marvell.com \
--cc=yuying.zhang@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).