Jesper Louis Andersen wrote:
>Quoting Branko ??ibej (brane@xbc.nu):
>
>
>>Please please read the section in HACKING about writing log messages.
>>
>>
>
>Bleh, sorry for my ignorance.
>
>
Ah, well... I decided to fix the remaining issues myself and committed
this in r11978. Good work, but please pay attention to the following
comments for next time.
>Indentation fixed,
>
>
not everywhere...
>Diff reduced against trunk by insterting and removing newlines where
> there were differences,
>Space inserted after function identifier, before () group,
>__ removed from code,
>static, APR_INLINE where applicable,
>first_non_whitespace*, and string_compare* functions common logic
> hoisted into helper functions.
>More tests for all changed functions,
>Correct log message (hopefully).
>
>
The log message is too verbose. You try to document the new functions in
the log message, but that isn't necessary -- they should be documented
in the code. I reduced you log message (although it's still a bit longer
than optimal); please look at it and compare to what's written in HACKING.
>+static APR_INLINE svn_boolean_t
>+string_compare (const char *str1,
>+ const char *str2,
>
>
You left trailing whitespace in a number of places, the first one right
here. It's a small nit, but it's a nit.
>@@ -326,6 +348,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;
>
>
Oops. There was a tab character on this line. This is a bigger nit than
the trailing whitespace.
>+ return test_find_char_backward (a->data,
>+ a->len,
>+ ',',
>+ a->len - 1,
>+ msg_only,
>+ pool);
>
>
The indentation is wrong here, and in several other places below. I hope
I found them all.
All in all, great work, I especially appreciate all the new tests. Needs
a wee bit more attention to detail, though... :-)
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 22 00:08:53 2004