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

Re: svn commit: r983807 - in /subversion/branches/atomic-revprop/subversion: include/svn_error.h libsvn_subr/error.c

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Tue, 10 Aug 2010 16:09:15 +0300

Julian Foad wrote on Tue, Aug 10, 2010 at 13:48:20 +0100:
> On Tue, 2010-08-10 at 15:04 +0300, Daniel Shahaf wrote:
> > I can see several options:
> >
> > * forbid passing SVN_NO_ERROR
> > * return FALSE on SVN_NO_ERROR
> > * reture (apr_err == APR_SUCCESS ? TRUE : FALSE) on SVN_NO_ERROR
> >
> > Right now, the API does the first and the code the third.
> >
> >
> > I suppose the question is (1) whether we expect the caller to test for
> > SVN_NO_ERROR before calling this function, and (2) whether we expect people to
> > pass APR_SUCCESS to this function. Personally my answers (while writing
> > this) were: (1) yes (2) no.
>
> Generally we have found that with error testing it's convenient to be
> able to write "err = foo(); do_something_with(err);" without having to
> check "err" is non-null first.

Yep, and I just realized that one of my pending patches also missed the "if
(err)" check. How about:

[[[
Index: subversion/libsvn_subr/error.c
===================================================================
--- subversion/libsvn_subr/error.c (revision 983929)
+++ subversion/libsvn_subr/error.c (working copy)
@@ -274,9 +274,8 @@
 {
   svn_error_t *child;
 
- if (! err && ! apr_err)
- /* The API doesn't specify the behaviour when ERR is NULL. */
- return TRUE;
+ if (err == SVN_NO_ERROR)
+ return FALSE;
   
   for (child = err; child; child = child->child)
     if (child->apr_err == apr_err)
Index: subversion/include/svn_error.h
===================================================================
--- subversion/include/svn_error.h (revision 983929)
+++ subversion/include/svn_error.h (working copy)
@@ -195,6 +195,8 @@
 
 /** Return TRUE if @a err's chain contains the error code @a apr_err.
  *
+ * If @a err is #SVN_NO_ERROR, return FALSE.
+ *
  * @since New in 1.7.
  */
 svn_boolean_t
]]]

> I agree with (2) - I don't think it makes much sense to pass APR_SUCCESS
> to this function.

Of course, but I don't think we should bother to special-case/check
for that.
Received on 2010-08-10 15:11:49 CEST

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