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