Re: [PATCH] Nitpick to libsvn_subr/svn_string.c

Quoting Branko ??ibej (brane@xbc.nu):

> I guess we could argue backwards and forwards and produce numbers that
> support both positions, while compilers get smarter and smarter... but I
> don't think it's worth it. Let's just fix the signedness issue you
> found, and leave it at that.

New patch. With the changes you suggested.

Beware that your

while(i != 0)
  {
    if (str[i--] == ch)
      return i;
...

will not return the char at position i, but the char at position i-1
due to the i--. The patch below for-loops it correctly (I, I love
regression checks).

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 = len; i != 0; i--)
+ {
+ if (str[i] == ch)
+ return 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,71 @@
   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, ',');
+
+ printf("%i\n", i);
+ 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 +540,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