Re: [PATCH] Replacement for "assert" in the libraries - issue #2780

I discovered that this is part of issue #2780, summary line "remove
abort() and assert() calls".

On Wed, 2008-06-18 at 16:01 +0100, Julian Foad wrote:
> The attached "assert-mech.patch" provides an assertion-checking macro
> that gives the application author control over how assertion failures
> are handled. This is intended to replace many instances of "assert()"
> and "abort()" in Subversion libraries. It allows the error to be
> propagated and handled through Subversion's svn_error_t cascading, and
> this is the default behaviour.

I changed the default behaviour to be "print to stderr, and then abort"
for backward compatibility. I removed the controversial line that
previously said the callback function could return SVN_NO_ERROR. I also
made all the naming much more self-consistent.

I have now checked in a version of this patch as r31931. I think there
was broad consensus that this will be an improvement. If I'm wrong about
that or it needs more work, let me know. I'd be particularly glad to
hear from GUI authors such as Mark Phippard and Steve King.

The log message is:

> Introduce a mechanism for reporting malfunctions in the Subversion libraries.
> This enables assertion failures and other "can't happen" scenarios to be
> detected with the simplicity of traditional "assert" and "abort" statements
> while also enabling these events to be caught and handled by the application
> if the application so chooses. The default behaviour is to print an error
> message to stderr and abort.
>
> This mechanism is suitable only for functions that return (svn_error_t *).
>
> This change adds the new mechanism but does not use it.
>
> This change covers part of issue #2780, "remove abort() and assert() calls".
>
> * subversion/include/svn_error_codes.h
> (SVN_ERR_MALFUNC_CATEGORY_START, SVN_ERR_ASSERTION_FAIL): New macros.
>
> * subversion/include/svn_error.h
> (SVN_ERR_MALFUNCTION, SVN_ERR_ASSERT): New macros.
> (svn_error__malfunction, svn_error_set_malfunction_handler,
> svn_error_raise_on_malfunction, svn_error_abort_on_malfunction):
> New functions.
> (svn_error_malfunction_handler_t): New type.
>
> * subversion/libsvn_subr/error.c
> (svn_error_raise_on_malfunction, svn_error_abort_on_malfunction,
> svn_error_set_malfunction_handler, svn_error__malfunction): New functions.
> (malfunction_handler): New variable.

I have briefly tested that the application can call
svn_error_set_malfunction_handler(svn_error_raise_on_malfunction) and
that an intermediate layer (svn_client_update3() in my test) can then
catch an assertion failure error and clear it and continue processing
other targets.

The attached "assert-conversion-1.patch" comprehensively converts all
possible abort()s and assert()s in the libraries
(subversion/libsvn_*/...) to use SVN_ERR_ASSERT(e) and
SVN_ERR_MALFUNCTION() instead. (Sorry it's rather long.)

I shall probably omit the "ra_serf" portion because many of ra_serf's
assertions and aborts need to be converted into input validation tests
instead and this conversion would just cause code churn in those cases.

Any comments before I proceed?

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org