[PATCH] Replacement for "assert" in the libraries

Re. the thread "[RFC] Replacement for "assert" in the libraries".

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.

There are in fact two macros:
    SVN_ERR_ASSERT(expr)
    SVN_ERR_MALFUNCTION()

The latter is (like "assert(FALSE)") to mark a place where the code flow
could not have reached other then because of a bug.

Unlike "assert()" and "abort()", the new macros cause the containing
function to return an svn_error_t object, so they can only be used
within functions that return svn_error_t objects.

Here are responses to concerns that I found raised in the previous
thread:

* "I do think that there is value in having a way of assert/aborting
from anywhere in the code (not just svn_error_t-returning functions)
which can't be ignored accidentally by forgetting to write SVN_ERR and
friends." [DG]

-> The traditional (standard) assert()/abort() achieves this, but I
think you also accept that in the libraries we need to make such errors
trappable by the calling program. My proposal solves this for the
functions where an error return is available. I have no solution for the
other functions, so they can continue to use the traditional
assert()/abort().

-> If you leak an error by forgetting to write SVN_ERR, we know a way of
catching this at program end time (by looking to see if any error
objects are still allocated in their pool). Also, the application can
configure SVN_ERR_ASSERT mechanism to always quit immediately (or to
invoke some other non-recoverable action) without relying on the error
returns to be propagated. We could also consider preventing
svn_error_clear() from silently clearing assertions, to avoid
accidentally ignoring them.

* Allowing the application to continue (rather than aborting
immediately) may be dangerous because undefined behaviour may lead to
data loss. [VL]

-> It MAY of course, but it's safer than the alternatives. Forcing the
application to quit immediately when any unexpected condition is
encountered is unacceptable behaviour for a library, and may cause loss
of the application's data. It's better to allow the application
programmer to control how the unexpected condition is handled. He can
choose to make it always quit immediately if that is best for that
particular program.

* Wouldn't it be better to have a different kind of assertion for
conditions which strongly indicate that memory has already been
corrupted? [VL]

-> That's an idea worth looking into. For now, we can use the
traditional assert() cases where people feel it's better to always
terminate immediately.

* "assert/abort generally gives good stack traces when run in gdb,
whereas it's trickier to track down the source of our error
objects." [DG/CMP]

-> By implementing our assertion mechanism such that the macro calls a
single function that raises all assertion-failure errors, you can put a
breakpoint on it or change it to do something like "abort" instead. See
svn_error__internal_err().

* On Windows, assert() only shows a message and doesn't invoke the
debugger like it does with seg-faults. [SK]

-> We developers have control over this. I expect one can configure the
development environment to do something more useful. If not, we can
re-define the assertion function to cause a seg-fault. See

The second attached patch, "assert-examples.patch", shows how
"assert(e)" and "abort()" statements can be converted to use
SVN_ERR_ASSERT(e) and SVN_ERR_MALFUNCTION() respectively.

Thoughts? Reviews?

- Julian

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