On Tue, 2010-08-10, Daniel Shahaf wrote:
> 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;
No need to implement this check as a special case: the general case loop
below already does the same.
> 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.
Agreed.
- Julian
Received on 2010-08-10 15:34:13 CEST