[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Branko Čibej <brane_at_xbc.nu>
Date: 2004-11-14 15:29:20 CET

Jesper Louis Andersen wrote:

>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++)
>
>
Surely you mistyped that?

>...
>
>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.
>
>
Yes. (Not that I'd ever expect strings to get so large, but correctness
is nice.)

>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.
>
>
Ah, but whilst "len - i" can be CSE'd, it cant' be avoided. You can
avoid the slowdown by continuing to use a countdown loop but shifting
the limits, thusly:

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

>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().
>
>
That's fine. I'd put an APR_INLINE in the declaration, too, and make the
function static.

>Also, I changed a bit of memcmp's to check explicitly for == 0.
>I am not sure if this is the projects standard practice.
>
>
HACKING doesn't say, but I think we more or less do what you did.

>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.
>
>
New tests are always nice. :-)

-- Brane

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Nov 14 15:29:05 2004

This is an archived mail posted to the Subversion Dev mailing list.