On Wed, Feb 14, 2024 at 2:07 PM Bruce Richardson
<bruce.richard...@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.march...@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
