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