[PATCH] eal: add helper to skip whitespaces

[PATCH] eal: add helper to skip whitespaces

[David Marchand]

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);
 	if (*p != '\0')
 		return -EINVAL;
 
diff --git a/app/test-eventdev/parser.c b/app/test-eventdev/parser.c
index bbea47b87d..6a6676784e 100644
--- a/app/test-eventdev/parser.c
+++ b/app/test-eventdev/parser.c
@@ -40,7 +40,7 @@ get_hex_val(char c)
 int
 parser_read_arg_bool(const char *p)
 {
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	int result = -EINVAL;
 
 	if (((p[0] == 'y') && (p[1] == 'e') && (p[2] == 's')) ||
@@ -67,7 +67,7 @@ parser_read_arg_bool(const char *p)
 		result = 0;
 	}
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 
 	if (p[0] != '\0')
 		return -EINVAL;
@@ -81,7 +81,7 @@ parser_read_uint64(uint64_t *value, const char *p)
 	char *next;
 	uint64_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (!isdigit(*p))
 		return -EINVAL;
 
@@ -107,7 +107,7 @@ parser_read_uint64(uint64_t *value, const char *p)
 		break;
 	}
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (*p != '\0')
 		return -EINVAL;
 
@@ -121,7 +121,7 @@ parser_read_int32(int32_t *value, const char *p)
 	char *next;
 	int32_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (!isdigit(*p))
 		return -EINVAL;
 
@@ -139,13 +139,13 @@ parser_read_uint64_hex(uint64_t *value, const char *p)
 	char *next;
 	uint64_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 
 	val = strtoul(p, &next, 16);
 	if (p == next)
 		return -EINVAL;
 
-	p = skip_white_spaces(next);
+	p = rte_str_skip_whitespaces(next);
 	if (*p != '\0')
 		return -EINVAL;
 
diff --git a/app/test-eventdev/parser.h b/app/test-eventdev/parser.h
index 3617872997..6976efe877 100644
--- a/app/test-eventdev/parser.h
+++ b/app/test-eventdev/parser.h
@@ -10,14 +10,6 @@
 
 #define PARSE_DELIMITER				" \f\n\r\t\v"
 
-#define skip_white_spaces(pos)			\
-({						\
-	__typeof__(pos) _p = (pos);		\
-	for ( ; isspace(*_p); _p++)		\
-		;				\
-	_p;					\
-})
-
 static inline size_t
 skip_digits(const char *src)
 {
diff --git a/app/test-mldev/parser.c b/app/test-mldev/parser.c
index 277dcaedb2..6fd47ec77b 100644
--- a/app/test-mldev/parser.c
+++ b/app/test-mldev/parser.c
@@ -12,6 +12,7 @@
 #include <string.h>
 
 #include <rte_common.h>
+#include <rte_string_fns.h>
 
 #include "parser.h"
 
@@ -52,7 +53,7 @@ get_hex_val(char c)
 int
 parser_read_arg_bool(const char *p)
 {
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	int result = -EINVAL;
 
 	if (((p[0] == 'y') && (p[1] == 'e') && (p[2] == 's')) ||
@@ -77,7 +78,7 @@ parser_read_arg_bool(const char *p)
 		result = 0;
 	}
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 
 	if (p[0] != '\0')
 		return -EINVAL;
@@ -91,7 +92,7 @@ parser_read_uint64(uint64_t *value, const char *p)
 	char *next;
 	uint64_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (!isdigit(*p))
 		return -EINVAL;
 
@@ -117,7 +118,7 @@ parser_read_uint64(uint64_t *value, const char *p)
 		break;
 	}
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (*p != '\0')
 		return -EINVAL;
 
@@ -131,7 +132,7 @@ parser_read_int32(int32_t *value, const char *p)
 	char *next;
 	int32_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if ((*p != '-') && (!isdigit(*p)))
 		return -EINVAL;
 
@@ -149,7 +150,7 @@ parser_read_int16(int16_t *value, const char *p)
 	char *next;
 	int16_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if ((*p != '-') && (!isdigit(*p)))
 		return -EINVAL;
 
@@ -167,13 +168,13 @@ parser_read_uint64_hex(uint64_t *value, const char *p)
 	char *next;
 	uint64_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 
 	val = strtoul(p, &next, 16);
 	if (p == next)
 		return -EINVAL;
 
-	p = skip_white_spaces(next);
+	p = rte_str_skip_whitespaces(next);
 	if (*p != '\0')
 		return -EINVAL;
 
diff --git a/app/test-mldev/parser.h b/app/test-mldev/parser.h
index 8b4207d0b0..b12b8f24c0 100644
--- a/app/test-mldev/parser.h
+++ b/app/test-mldev/parser.h
@@ -12,14 +12,6 @@
 
 #define PARSE_DELIMITER " \f\n\r\t\v"
 
-#define skip_white_spaces(pos) \
-	({ \
-		__typeof__(pos) _p = (pos); \
-		for (; isspace(*_p); _p++) \
-			; \
-		_p; \
-	})
-
 static inline size_t
 skip_digits(const char *src)
 {
diff --git a/app/test-pmd/cmdline_tm.c b/app/test-pmd/cmdline_tm.c
index c11c80b158..2abd494ae9 100644
--- a/app/test-pmd/cmdline_tm.c
+++ b/app/test-pmd/cmdline_tm.c
@@ -11,6 +11,7 @@
 
 #include <rte_ethdev.h>
 #include <rte_flow.h>
+#include <rte_string_fns.h>
 #include <rte_tm.h>
 
 #include "testpmd.h"
@@ -19,14 +20,6 @@
 #define PARSE_DELIMITER				" \f\n\r\t\v"
 #define MAX_NUM_SHARED_SHAPERS		256
 
-#define skip_white_spaces(pos)			\
-({						\
-	__typeof__(pos) _p = (pos);		\
-	for ( ; isspace(*_p); _p++)		\
-		;				\
-	_p;					\
-})
-
 /** Display TM Error Message */
 static void
 print_err_msg(struct rte_tm_error *error)
@@ -112,7 +105,7 @@ read_uint64(uint64_t *value, const char *p)
 	char *next;
 	uint64_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (!isdigit(*p))
 		return -EINVAL;
 
@@ -138,7 +131,7 @@ read_uint64(uint64_t *value, const char *p)
 		break;
 	}
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (*p != '\0')
 		return -EINVAL;
 
diff --git a/app/test/test_string_fns.c b/app/test/test_string_fns.c
index ad41106df1..d87ee04483 100644
--- a/app/test/test_string_fns.c
+++ b/app/test/test_string_fns.c
@@ -172,6 +172,39 @@ test_rte_strlcat(void)
 	return 0;
 }
 
+static int
+test_rte_str_skip_whitespaces(void)
+{
+	static const char empty[] = "";
+	static const char nowhitespace[] = "Thereisreallynowhitespace";
+	static const char somewhitespaces[] = " \f\n\r\t\vThere are some whitespaces";
+	const char *p;
+
+	LOG("Checking '%s'\n", empty);
+	p = rte_str_skip_whitespaces(empty);
+	if (p != empty) {
+		LOG("Returned address '%s' does not match expected result\n", p);
+		return -1;
+	}
+	LOG("Got expected '%s'\n", p);
+	LOG("Checking '%s'\n", nowhitespace);
+	p = rte_str_skip_whitespaces(nowhitespace);
+	if (p != nowhitespace) {
+		LOG("Returned address '%s' does not match expected result\n", p);
+		return -1;
+	}
+	LOG("Got expected '%s'\n", p);
+	LOG("Checking '%s'\n", somewhitespaces);
+	p = rte_str_skip_whitespaces(somewhitespaces);
+	if (p != strchr(somewhitespaces, 'T')) {
+		LOG("Returned address '%s' does not match expected result\n", p);
+		return -1;
+	}
+	LOG("Got expected '%s'\n", p);
+
+	return 0;
+}
+
 static int
 test_string_fns(void)
 {
@@ -179,6 +212,8 @@ test_string_fns(void)
 		return -1;
 	if (test_rte_strlcat() < 0)
 		return -1;
+	if (test_rte_str_skip_whitespaces() < 0)
+		return -1;
 	return 0;
 }
 
diff --git a/examples/fips_validation/fips_validation.c b/examples/fips_validation/fips_validation.c
index f840804009..15432b75c3 100644
--- a/examples/fips_validation/fips_validation.c
+++ b/examples/fips_validation/fips_validation.c
@@ -13,14 +13,6 @@
 
 #include "fips_validation.h"
 
-#define skip_white_spaces(pos)			\
-({						\
-	__typeof__(pos) _p = (pos);		\
-	for ( ; isspace(*_p); _p++)		\
-		;				\
-	_p;					\
-})
-
 static int
 get_file_line(void)
 {
@@ -579,13 +571,13 @@ parser_read_uint64_hex(uint64_t *value, const char *p)
 	char *next;
 	uint64_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 
 	val = strtoul(p, &next, 16);
 	if (p == next)
 		return -EINVAL;
 
-	p = skip_white_spaces(next);
+	p = rte_str_skip_whitespaces(next);
 	if (*p != '\0')
 		return -EINVAL;
 
@@ -759,7 +751,7 @@ parser_read_uint64(uint64_t *value, const char *p)
 	char *next;
 	uint64_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (!isdigit(*p))
 		return -EINVAL;
 
@@ -785,7 +777,7 @@ parser_read_uint64(uint64_t *value, const char *p)
 		break;
 	}
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (*p != '\0')
 		return -EINVAL;
 
diff --git a/examples/ip_pipeline/parser.c b/examples/ip_pipeline/parser.c
index dfd71a71d3..b7cec4dd4c 100644
--- a/examples/ip_pipeline/parser.c
+++ b/examples/ip_pipeline/parser.c
@@ -42,7 +42,7 @@ get_hex_val(char c)
 int
 parser_read_arg_bool(const char *p)
 {
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	int result = -EINVAL;
 
 	if (((p[0] == 'y') && (p[1] == 'e') && (p[2] == 's')) ||
@@ -69,7 +69,7 @@ parser_read_arg_bool(const char *p)
 		result = 0;
 	}
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 
 	if (p[0] != '\0')
 		return -EINVAL;
@@ -83,7 +83,7 @@ parser_read_uint64(uint64_t *value, const char *p)
 	char *next;
 	uint64_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (!isdigit(*p))
 		return -EINVAL;
 
@@ -109,7 +109,7 @@ parser_read_uint64(uint64_t *value, const char *p)
 		break;
 	}
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (*p != '\0')
 		return -EINVAL;
 
@@ -123,13 +123,13 @@ parser_read_uint64_hex(uint64_t *value, const char *p)
 	char *next;
 	uint64_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 
 	val = strtoul(p, &next, 16);
 	if (p == next)
 		return -EINVAL;
 
-	p = skip_white_spaces(next);
+	p = rte_str_skip_whitespaces(next);
 	if (*p != '\0')
 		return -EINVAL;
 
@@ -420,7 +420,7 @@ parse_cpu_core(const char *entry,
 
 	uint32_t s = 0, c = 0, h = 0, val;
 	uint8_t s_parsed = 0, c_parsed = 0, h_parsed = 0;
-	const char *next = skip_white_spaces(entry);
+	const char *next = rte_str_skip_whitespaces(entry);
 	char type;
 
 	if (p == NULL)
diff --git a/examples/ip_pipeline/parser.h b/examples/ip_pipeline/parser.h
index 5224b18a89..32b5ff9976 100644
--- a/examples/ip_pipeline/parser.h
+++ b/examples/ip_pipeline/parser.h
@@ -13,14 +13,6 @@
 
 #define PARSE_DELIMITER				" \f\n\r\t\v"
 
-#define skip_white_spaces(pos)			\
-({						\
-	__typeof__(pos) _p = (pos);		\
-	for ( ; isspace(*_p); _p++)		\
-		;				\
-	_p;					\
-})
-
 static inline size_t
 skip_digits(const char *src)
 {
diff --git a/examples/pipeline/cli.c b/examples/pipeline/cli.c
index 2ae6cc579f..d85772308c 100644
--- a/examples/pipeline/cli.c
+++ b/examples/pipeline/cli.c
@@ -19,6 +19,7 @@
 #include <rte_swx_pipeline.h>
 #include <rte_swx_ctl.h>
 #include <rte_swx_ipsec.h>
+#include <rte_string_fns.h>
 
 #include "cli.h"
 
@@ -45,21 +46,13 @@
 #define MSG_FILE_NOT_ENOUGH "Not enough rules in file \"%s\".\n"
 #define MSG_CMD_FAIL        "Command \"%s\" failed.\n"
 
-#define skip_white_spaces(pos)			\
-({						\
-	__typeof__(pos) _p = (pos);		\
-	for ( ; isspace(*_p); _p++)		\
-		;				\
-	_p;					\
-})
-
 static int
 parser_read_uint64(uint64_t *value, const char *p)
 {
 	char *next;
 	uint64_t val;
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (!isdigit(*p))
 		return -EINVAL;
 
@@ -85,7 +78,7 @@ parser_read_uint64(uint64_t *value, const char *p)
 		break;
 	}
 
-	p = skip_white_spaces(p);
+	p = rte_str_skip_whitespaces(p);
 	if (*p != '\0')
 		return -EINVAL;
 
diff --git a/lib/eal/include/rte_string_fns.h b/lib/eal/include/rte_string_fns.h
index bb43b2cba3..a9c9308667 100644
--- a/lib/eal/include/rte_string_fns.h
+++ b/lib/eal/include/rte_string_fns.h
@@ -15,10 +15,12 @@
 extern "C" {
 #endif
 
+#include <ctype.h>
 #include <stdio.h>
 #include <string.h>
 
 #include <rte_common.h>
+#include <rte_compat.h>
 
 /**
  * Takes string "string" parameter and splits it at character "delim"
@@ -115,6 +117,31 @@ rte_strlcat(char *dst, const char *src, size_t size)
 ssize_t
 rte_strscpy(char *dst, const char *src, size_t dsize);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Search for the first non-whitespace character.
+ *
+ * @param src
+ *   The input string to be analysed.
+ *
+ * @return
+ *  The address of the first non whitespace character.
+ */
+__rte_experimental
+static inline const char *
+rte_str_skip_whitespaces(const char *src)
+{
+	const char *p = src;
+
+	while (isspace(*p))
+		p++;
+
+	return p;
+}
+
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.43.0

Re: [PATCH] eal: add helper to skip whitespaces

[Bruce Richardson]

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.

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.

/Bruce

Re: [PATCH] eal: add helper to skip whitespaces

[David Marchand]

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

Re: [PATCH] eal: add helper to skip whitespaces

[Bruce Richardson]

On Thu, Feb 15, 2024 at 10:08:40AM +0100, David Marchand wrote:
> 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.
> 

Ok, good point. I'd suggest otherwise just going with your patch, perhaps
with the tweaked name.

/Bruce

Re: [PATCH] eal: add helper to skip whitespaces

[Tyler Retzlaff]

On Thu, Feb 15, 2024 at 09:10:58AM +0000, Bruce Richardson wrote:
> On Thu, Feb 15, 2024 at 10:08:40AM +0100, David Marchand wrote:
> > 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.
> > 
> 
> Ok, good point. I'd suggest otherwise just going with your patch, perhaps
> with the tweaked name.
> 
> /Bruce

whatever name is chosen I appreciate the patch. Also, this patch saves
me from doing the same or something else later because all the
duplicated macros exapnd to statement expressions.

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>