Jesper Louis Andersen wrote:
>Quoting Branko ??ibej (brane@xbc.nu):
>
>
>>You misread what I posted.
>>
>>
>Oops!
>
>Yes, I see the point. Here is what I have done:
>
>First, I expanded the tests so we now test 5 equivalence classes:
> * The character is in the middle of the string
> * The character is at the very beginning of the string
> * The character is at the very end of the string
> * The character is not in the string at all
> * The string has len 0
>
>
Very good.
>Next, I verified that my implementation fails. It does ;)
>
>
Heh.
>Then, I changed to your implementation, correctly this time, and
> verified it works.
>
>
Good.
>--- Log Message ---
>
>
Please please read the section in HACKING about writing log messages.
You can get good examples from the Subversion logs, e.g., by running
svn log http://svn.collab.net/repos/svn/trunk/subversion/include
There are also still a few formatting probems in your patch. Please read
the relevant sections in HACKING carefully, and try to match your style
to the surrounding code.
Sorry if all this seems pedantic, but we really must insist on following
our coding standatds, because it makes the code easier to read and
maintain. Certainly it means that patch submissions are more work, but
in the long run, a bit of coding discipline is all for the best. :-)
>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,21 @@
> return new_string;
> }
>
>+apr_size_t
>+string__find_char_backward (const char *str, apr_size_t len, char ch)
>
>
As I said before, this function should be declared static. The name
shouldn't contain a double underscore (that's reserved for non-static
function that aren't exported from the library). I suggest
static APR_INLINE apr_size_t
find_char_backward (...
>Index: subversion/tests/libsvn_subr/string-test.c
>
>
This file has the space before parens in function calls. You should
follow the style in use by the file.
>@@ -57,8 +58,6 @@
> const char *phrase_2 = "a longish phrase of sorts, longer than 16 anyway";
>
>
>-
>-
>
>
Don't make unnecessary whitespace changes, they just make the diff bigger.
> static svn_error_t *
> test1 (const char **msg,
> svn_boolean_t msg_only,
>@@ -445,7 +444,124 @@
> return SVN_NO_ERROR;
> }
>
>+/* Helper function for checking correctness of find_char_backward */
>+static svn_error_t *
>+test__find_char_backward(const char* data,
>
>
No double underscore in the name, and missing space before paren.
>+static svn_error_t *
>+test13 (const char **msg,
>+ svn_boolean_t msg_only,
>+ apr_pool_t *pool)
>+{
>+ *msg = "find_char_backward; middle case";
>+ a = svn_stringbuf_create("test, test", pool);
>
>
Missing space before paren.
>+
>+ return
>+ test__find_char_backward(a->data, a->len, ',', 4, msg_only, pool);
>
>
And here... I won't mention all the other places, there are too many.
>+static svn_error_t *
>+test16 (const char **msg,
>+ svn_boolean_t msg_only,
>+ apr_pool_t *pool)
>+{
>+ *msg = "find_char_backward; len = 0 case";
>+
>+ a = svn_stringbuf_create("", pool);
>+
>+ assert(svn_stringbuf_isempty(a));
>
>
This assert is unnecessary. If after all the other tests in this file
you still don't believe that svn_stringbuf_create with an empty string
parameter works correctly, then add another test case for that.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 15 11:49:46 2004