On 12/23/10 3:01 AM, Daniel Shahaf wrote:
> I've made a quick sketch --- looks good?
The suggestions look good.
We could add some more tests, checking the behavior with SVN_NO_ERROR input and
one where all errors are tracing, which should return an error from
SVN_ERR_ASSERT().
As an aside, there's a comment about adding an svn_boolean_t to svn_error_t
instead of using strcmp(), which could be done in a totally separate commit.
>> Index: subversion/mod_dav_svn/deadprops.c
>> ===================================================================
>> --- subversion/mod_dav_svn/deadprops.c (revision 1052167)
>> +++ subversion/mod_dav_svn/deadprops.c (working copy)
>> @@ -222,18 +222,22 @@
>> + purged_serr->message = apr_xml_quote_string
>> + (purged_serr->pool,
>
> Our current policy is to have the '(' on the same line as the function name.
I looked through the hacking guide and didn't see anything, but doing some greps
through our code shows a disposition to this style:
$ cd subversion/
$ grep '^ *(' */*.c| wc -l
1757
$ grep '($' */*.c| wc -l
454
>> Index: subversion/mod_dav_svn/version.c
>> ===================================================================
>> --- subversion/mod_dav_svn/version.c (revision 1052167)
>> +++ subversion/mod_dav_svn/version.c (working copy)
>> @@ -922,16 +922,16 @@
>> {
>> if (serr)
>> {
>> - const char *post_commit_err;
>> - apr_err = serr->apr_err;
>> - post_commit_err = svn_repos__post_commit_error_str
>> - (serr, resource->pool);
>> - serr = SVN_NO_ERROR;
>> - ap_log_perror(APLOG_MARK, APLOG_ERR, apr_err, resource->pool,
>> + const char *post_commit_err = svn_repos__post_commit_error_str
>> + (serr, resource->pool);
>> + ap_log_perror(APLOG_MARK, APLOG_ERR, serr->apr_err,
>> + resource->pool,
>
> The rest looks good. (I am assuming --- didn't check --- that you
> updated all callers.)
Yes.
Blair
Received on 2010-12-24 02:11:04 CET