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.