[PATCH] memmem.c, strstr.c: use unsigned int instead of size_t for small needles

[PATCH] memmem.c, strstr.c: use unsigned int instead of size_t for small needles

[James Tirta Halim]

---
 string/memmem.c | 10 +++++++---
 string/strstr.c | 10 +++++++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/string/memmem.c b/string/memmem.c
index 13df3bcdd4..d5548e5f71 100644
--- a/string/memmem.c
+++ b/string/memmem.c
@@ -80,9 +80,13 @@ __memmem (const void *haystack, size_t hs_len,
     return two_way_long_needle (hs, hs_len, ne, ne_len);
 
   uint8_t shift[256];
-  size_t tmp, shift1;
-  size_t m1 = ne_len - 1;
-  size_t offset = 0;
+
+  typedef unsigned int Idx;
+  _Static_assert (sizeof (shift) / sizeof (shift[0]) == 256 && sizeof (shift[0]) <= sizeof (Idx), "Index type is too small.");
+
+  Idx tmp, shift1;
+  Idx m1 = ne_len - 1;
+  Idx offset = 0;
 
   memset (shift, 0, sizeof (shift));
   for (int i = 1; i < m1; i++)
diff --git a/string/strstr.c b/string/strstr.c
index 23618e2eb2..132a115214 100644
--- a/string/strstr.c
+++ b/string/strstr.c
@@ -108,9 +108,13 @@ STRSTR (const char *haystack, const char *needle)
 
   const unsigned char *end = hs + hs_len - ne_len;
   uint8_t shift[256];
-  size_t tmp, shift1;
-  size_t m1 = ne_len - 1;
-  size_t offset = 0;
+
+  typedef unsigned int Idx;
+  _Static_assert (sizeof (shift) / sizeof (shift[0]) == 256 && sizeof (shift[0]) <= sizeof (Idx), "Index type is too small.");
+
+  Idx tmp, shift1;
+  Idx m1 = ne_len - 1;
+  Idx offset = 0;
 
   /* Initialize bad character shift hash table.  */
   memset (shift, 0, sizeof (shift));
-- 
2.43.0

[PATCH] memmem.c, strstr.c: use unsigned int instead of size_t for small needles

[Wilco Dijkstra]

Hi James,

What is the goal of this patch? Is there a performance difference?
You should provide a clear description that explains the reasons
behind this patch and the benefits - without this, it is unlikely to be
accepted.

> +  typedef unsigned int Idx;

Why create a new typedef when we already have uint32_t?

> +  _Static_assert (sizeof (shift) / sizeof (shift[0]) == 256 && sizeof (shift[0]) <= sizeof (Idx), "Index type is too small.");

What is the point of this just a few lines below the definition of shift?
It's obvious that a 32-bit type will work - but what isn't obvious is why
you think that is better than using size_t.

Cheers,
Wilco

Re: [PATCH] memmem.c, strstr.c: use unsigned int instead of size_t for small needles

[Carlos O'Donell]

On 12/14/23 09:20, James Tirta Halim wrote:
> ---
>  string/memmem.c | 10 +++++++---
>  string/strstr.c | 10 +++++++---
>  2 files changed, 14 insertions(+), 6 deletions(-)

The feedback at this point is that these kinds of activities need performance
microbenchmark data to show the benefit across target machines.

Likewise this needs copyright assignment or DCO.

Thank you!

> 
> diff --git a/string/memmem.c b/string/memmem.c
> index 13df3bcdd4..d5548e5f71 100644
> --- a/string/memmem.c
> +++ b/string/memmem.c
> @@ -80,9 +80,13 @@ __memmem (const void *haystack, size_t hs_len,
>      return two_way_long_needle (hs, hs_len, ne, ne_len);
>  
>    uint8_t shift[256];
> -  size_t tmp, shift1;
> -  size_t m1 = ne_len - 1;
> -  size_t offset = 0;
> +
> +  typedef unsigned int Idx;
> +  _Static_assert (sizeof (shift) / sizeof (shift[0]) == 256 && sizeof (shift[0]) <= sizeof (Idx), "Index type is too small.");
> +
> +  Idx tmp, shift1;
> +  Idx m1 = ne_len - 1;
> +  Idx offset = 0;
>  
>    memset (shift, 0, sizeof (shift));
>    for (int i = 1; i < m1; i++)
> diff --git a/string/strstr.c b/string/strstr.c
> index 23618e2eb2..132a115214 100644
> --- a/string/strstr.c
> +++ b/string/strstr.c
> @@ -108,9 +108,13 @@ STRSTR (const char *haystack, const char *needle)
>  
>    const unsigned char *end = hs + hs_len - ne_len;
>    uint8_t shift[256];
> -  size_t tmp, shift1;
> -  size_t m1 = ne_len - 1;
> -  size_t offset = 0;
> +
> +  typedef unsigned int Idx;
> +  _Static_assert (sizeof (shift) / sizeof (shift[0]) == 256 && sizeof (shift[0]) <= sizeof (Idx), "Index type is too small.");
> +
> +  Idx tmp, shift1;
> +  Idx m1 = ne_len - 1;
> +  Idx offset = 0;
>  
>    /* Initialize bad character shift hash table.  */
>    memset (shift, 0, sizeof (shift));

-- 
Cheers,
Carlos.