bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] (x)memcoll: minor tweaks


From: Paul Eggert
Subject: Re: [PATCH] (x)memcoll: minor tweaks
Date: Mon, 12 Jul 2010 11:16:38 -0700
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.10) Gecko/20100527 Thunderbird/3.0.5

On 07/11/10 05:06, Bruno Haible wrote:

> There is one API bug and two oddities in your patch:

Thanks for your comments.  The patch below tries to address them,
although perhaps not always entirely in the way you asked.  The
documentation was misleading: the new function's size arguments count
the NUL bytes.  This distinguishes it from the old function, which
uses lengths not sizes.  So there is no loop overrun.  I installed the
following patch to try to make this clearer, and also to make the code
a bit smaller and faster while I was at it.  It may be OK to add back
the abort() tests, but I'd like to see the performance figures first:
it's not simply the instruction count, it's also the extra pressure on
the code and data caches.  In some cases (e.g., one string is empty,
or this locale uses no special collation rules) strcoll need not fire
up the locale machinery and might run pretty fast.


>From 444cc405e1ad5afea49c9757be5a2c31c81804b6 Mon Sep 17 00:00:00 2001
From: Paul R. Eggert <address@hidden>
Date: Mon, 12 Jul 2010 10:58:35 -0700
Subject: [PATCH] memcoll: clarify sizes versus lengths, document better, and 
tweak perf

* lib/memcoll.c (strcoll_loop, memcoll0):
Improve quality of descriptive comments.  Name variables
consistently as to whether they are lengths (which do not include
terminating null) versus sizes (which do).
* lib/xmemcoll.c (xmemcoll0): Likewise.
* lib/memcoll.c (strcoll_loop): Tweak the way that the diff is
returned when s1size == 0; this is easier to compile and saves
about 17% of memcoll's code space on x86-64 with GCC 4.1.2.
---
 ChangeLog      |   12 ++++++++++++
 lib/memcoll.c  |   52 ++++++++++++++++++++++++----------------------------
 lib/xmemcoll.c |    6 ++++--
 3 files changed, 40 insertions(+), 30 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 0034cad..886f5bd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2010-07-12  Paul R. Eggert  <address@hidden>
+
+       memcoll: clarify sizes versus lengths, document better, and tweak perf
+       * lib/memcoll.c (strcoll_loop, memcoll0):
+       Improve quality of descriptive comments.  Name variables
+       consistently as to whether they are lengths (which do not include
+       terminating null) versus sizes (which do).
+       * lib/xmemcoll.c (xmemcoll0): Likewise.
+       * lib/memcoll.c (strcoll_loop): Tweak the way that the diff is
+       returned when s1size == 0; this is easier to compile and saves
+       about 17% of memcoll's code space on x86-64 with GCC 4.1.2.
+
 2010-07-12  Bruno Haible  <address@hidden>
 
        Tests for module '_Exit'.
diff --git a/lib/memcoll.c b/lib/memcoll.c
index 580f81d..39b383c 100644
--- a/lib/memcoll.c
+++ b/lib/memcoll.c
@@ -26,11 +26,13 @@
 #include <stdlib.h>
 #include <string.h>
 
-/* Ensure strcoll operates on the entire input strings, in case they contain
-   NUL bytes. */
-
+/* Compare S1 (with size S1SIZE) and S2 (with length S2SIZE) according
+   to the LC_COLLATE locale.  S1 and S2 are both blocks of memory with
+   nonzero sizes, and the last byte in each block must be a null byte.
+   Set errno to an error number if there is an error, and to zero
+   otherwise.  */
 static inline int
-strcoll_loop (char const *s1, size_t s1len, const char *s2, size_t s2len)
+strcoll_loop (char const *s1, size_t s1size, char const *s2, size_t s2size)
 {
   int diff;
 
@@ -43,20 +45,13 @@ strcoll_loop (char const *s1, size_t s1len, const char *s2, 
size_t s2len)
       size_t size2 = strlen (s2) + 1;
       s1 += size1;
       s2 += size2;
-      s1len -= size1;
-      s2len -= size2;
-
-      if (s1len == 0)
-        {
-          if (s2len != 0)
-            diff = -1;
-          break;
-        }
-      else if (s2len == 0)
-        {
-          diff = 1;
-          break;
-        }
+      s1size -= size1;
+      s2size -= size2;
+
+      if (s1size == 0)
+        return - (s2size != 0);
+      if (s2size == 0)
+        return 1;
     }
 
   return diff;
@@ -86,30 +81,31 @@ memcoll (char *s1, size_t s1len, char *s2, size_t s2len)
       char n1 = s1[s1len];
       char n2 = s2[s2len];
 
-      s1[s1len++] = '\0';
-      s2[s2len++] = '\0';
+      s1[s1len] = '\0';
+      s2[s2len] = '\0';
 
-      diff = strcoll_loop (s1, s1len, s2, s2len);
+      diff = strcoll_loop (s1, s1len + 1, s2, s2len + 1);
 
-      s1[s1len - 1] = n1;
-      s2[s2len - 1] = n2;
+      s1[s1len] = n1;
+      s2[s2len] = n2;
     }
 
   return diff;
 }
 
-/* Compare S1 (with length S1LEN) and S2 (with length S2LEN) according
-   to the LC_COLLATE locale.  S1 and S2 must both end in a null byte.
+/* Compare S1 (with size S1SIZE) and S2 (with length S2SIZE) according
+   to the LC_COLLATE locale.  S1 and S2 are both blocks of memory with
+   nonzero sizes, and the last byte in each block must be a null byte.
    Set errno to an error number if there is an error, and to zero
    otherwise.  */
 int
-memcoll0 (char const *s1, size_t s1len, const char *s2, size_t s2len)
+memcoll0 (char const *s1, size_t s1size, char const *s2, size_t s2size)
 {
-  if (s1len == s2len && memcmp (s1, s2, s1len) == 0)
+  if (s1size == s2size && memcmp (s1, s2, s1size) == 0)
     {
       errno = 0;
       return 0;
     }
   else
-    return strcoll_loop (s1, s1len, s2, s2len);
+    return strcoll_loop (s1, s1size, s2, s2size);
 }
diff --git a/lib/xmemcoll.c b/lib/xmemcoll.c
index 7f8c894..d2ddc33 100644
--- a/lib/xmemcoll.c
+++ b/lib/xmemcoll.c
@@ -60,8 +60,10 @@ xmemcoll (char *s1, size_t s1len, char *s2, size_t s2len)
   return diff;
 }
 
-/* Like xmemcoll, but S1 and S2 are known to be NUL delimited, thus
-   no modifications to S1 and S2 are needed. */
+/* Compare S1 (with size S1SIZE) and S2 (with length S2SIZE) according
+   to the LC_COLLATE locale.  S1 and S2 are both blocks of memory with
+   nonzero sizes, and the last byte in each block must be a null byte.
+   Report an error and exit if there is an error.  */
 
 int
 xmemcoll0 (char const *s1, size_t s1len, char const *s2, size_t s2len)
-- 
1.7.1




reply via email to

[Prev in Thread] Current Thread [Next in Thread]