You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@subversion.apache.org by Jesper Louis Andersen <jl...@mongers.org> on 2004/11/14 13:43:05 UTC

[PATCH] Nitpick to libsvn_subr/svn_string.c

While reading through the source of svn_string.c I found a little,
tiny nitpick. *_find_char_backward uses a signed int i, because this
i will get negative in the for loop. The problem is the initial
assignment in the for-loop in svn_string.c a bit after line 444:

for (i = (str->len -1); i >= 0; i++)
...

Now, str->len is of type apr_size_t, which is unsigned. So running 
this on a string big enough makes i negative by a big amount and 
the for-loop never executes and returns str->len. This is wrong.

The patch simply works by making i an apr_size_t, increasing it
and making the calculation str->len - i instead. Note that str->len
is loop invariant and str->len - i can be common subexpression 
eliminated. Thus the performance drop should not be measureable.

svn_string_t and svn_stringbuf_t uses identical code for 
string_find_char_backwards. Furthermore the functions are pure.
Hoist common logic into a new function string_find_char_backwards().

Also, I changed a bit of memcmp's to check explicitly for == 0.
I am not sure if this is the projects standard practice.

While we are here, add tests to the string tests to ensure this
new behaviour is as the old behaviour was. Also add a test for
whitespace stripping a svn_stringbuf_t.

Patch follows. Passes (new) regression suite:


Index: subversion/libsvn_subr/svn_string.c
===================================================================
--- subversion/libsvn_subr/svn_string.c	(revision 11892)
+++ subversion/libsvn_subr/svn_string.c	(working copy)
@@ -71,6 +71,20 @@
   return new_string;
 }
 
+apr_size_t
+string_find_char_backward (const char *str, apr_size_t len, char ch)
+{
+  apr_size_t i;
+
+  for (i = 0; i <= len; i++)
+    {
+      if (str[len - i] == ch)
+        return (len - i);
+    }
+  
+  return len;
+}
+
 svn_string_t *
 svn_string_ncreate (const char *bytes, apr_size_t size, apr_pool_t *pool)
 {
@@ -152,10 +166,10 @@
 
   /* now that we know they have identical lengths... */
   
-  if (memcmp (str1->data, str2->data, str1->len))
-    return FALSE;
-  else
+  if ((memcmp (str1->data, str2->data, str1->len)) == 0)
     return TRUE;
+  else
+    return FALSE;
 }
 
 
@@ -177,22 +191,12 @@
   return str->len;
 }
 
-
 apr_size_t
 svn_string_find_char_backward (const svn_string_t *str, char ch)
 {
-  int i;        /* signed! */
-
-  for (i = (str->len - 1); i >= 0; i--)
-    {
-      if (str->data[i] == ch)
-        return i;
-    }
-
-  return str->len;
+  return string_find_char_backward(str->data, str->len, ch);
 }
 
-
 
 /* svn_stringbuf functions */
 
@@ -326,6 +330,7 @@
           {
             apr_size_t prev_size = str->blocksize;
             str->blocksize *= 2;
+	    /* check for apr_size_t overflow */
             if (prev_size > str->blocksize)
               {
                 str->blocksize = minimum_size;
@@ -401,10 +406,10 @@
 
   /* now that we know they have identical lengths... */
   
-  if (memcmp (str1->data, str2->data, str1->len))
-    return FALSE;
-  else
+  if ((memcmp (str1->data, str2->data, str1->len)) == 0)
     return TRUE;
+  else
+    return FALSE;
 }
 
 
@@ -448,18 +453,9 @@
 apr_size_t
 svn_stringbuf_find_char_backward (const svn_stringbuf_t *str, char ch)
 {
-  int i;        /* signed! */
-
-  for (i = (str->len - 1); i >= 0; i--)
-    {
-      if (str->data[i] == ch)
-        return i;
-    }
-
-  return str->len;
+  return string_find_char_backward (str->data, str->len, ch);
 }
 
-
 svn_boolean_t
 svn_string_compare_stringbuf (const svn_string_t *str1,
                               const svn_stringbuf_t *str2)
@@ -470,10 +466,10 @@
 
   /* now that we know they have identical lengths... */
 
-  if (memcmp (str1->data, str2->data, str1->len))
-    return FALSE;
-  else
+  if ((memcmp (str1->data, str2->data, str1->len)) == 0)
     return TRUE;
+  else
+    return FALSE;
 }
 
 
Index: subversion/tests/libsvn_subr/string-test.c
===================================================================
--- subversion/tests/libsvn_subr/string-test.c	(revision 11892)
+++ subversion/tests/libsvn_subr/string-test.c	(working copy)
@@ -56,9 +56,16 @@
 const char *phrase_1 = "hello, ";
 const char *phrase_2 = "a longish phrase of sorts, longer than 16 anyway";
 
+/* the position of the comma in phrase_2 */
+const apr_size_t phrase_2_comma_postion = 25;
 
 
+const char *phrase_3 = "test";
+const char *phrase_3_whitespace = "   \ttest\t\t   ";
 
+
+
+
 static svn_error_t *
 test1 (const char **msg, 
        svn_boolean_t msg_only,
@@ -445,7 +452,70 @@
   return SVN_NO_ERROR;
 }
 
+static svn_error_t *
+test13 (const char **msg,
+	svn_boolean_t msg_only,
+	apr_pool_t *pool)
+{
+  apr_size_t i;
 
+  *msg = "find a character backwards in a svn_stringbuf_t";
+
+  if (msg_only)
+    return SVN_NO_ERROR;
+
+  a = svn_stringbuf_create (phrase_2, pool);
+  i = svn_stringbuf_find_char_backward (a, ',');
+
+  if (i == phrase_2_comma_postion)
+    return SVN_NO_ERROR;
+  else
+    return fail (pool, "test failed");
+}
+  
+static svn_error_t *
+test14 (const char **msg,
+	svn_boolean_t msg_only,
+	apr_pool_t *pool)
+{
+  apr_size_t i;
+
+  *msg = "find a character backwards in a svn_string_t";
+
+  if (msg_only)
+    return SVN_NO_ERROR;
+
+  a = svn_string_create (phrase_2, pool);
+  i = svn_string_find_char_backward (a, ',');
+
+  if (i == phrase_2_comma_postion)
+    return SVN_NO_ERROR;
+  else
+    return fail (pool, "test failed");
+}
+
+static svn_error_t *
+test15 (const char **msg,
+	svn_boolean_t msg_only,
+	apr_pool_t *pool)
+{
+  *msg = "check that whitespace will be stripped correctly";
+
+  if (msg_only)
+    return SVN_NO_ERROR;
+
+  a = svn_stringbuf_create (phrase_3_whitespace, pool);
+  b = svn_stringbuf_create (phrase_3, pool);
+
+  svn_stringbuf_strip_whitespace(a);
+
+  if (svn_stringbuf_compare(a, b) == TRUE)
+    return SVN_NO_ERROR;
+  else
+    return fail (pool, "test failed");
+}
+  
+
 /*
    ====================================================================
    If you add a new test to this file, update this array.
@@ -469,5 +539,8 @@
     SVN_TEST_PASS (test10),
     SVN_TEST_PASS (test11),
     SVN_TEST_PASS (test12),
+    SVN_TEST_PASS (test13),
+    SVN_TEST_PASS (test14),
+    SVN_TEST_PASS (test15),
     SVN_TEST_NULL
   };

-- 
jlouis

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org