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

[Julian Foad <ju...@btopenworld.com>]

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

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

["Hyrum K. Wright" <hy...@mail.utexas.edu>]

Mark Phippard wrote:
> It'd be great if whatever we did with this new change allowed the
> JavaHL C++ library to turn it into a Java exception.  That might be
> hoping for too much though.

I can't make any promises, but if somebody bugs me once this stuff gets 
hammered out in the C code, I'll see what I can do for JavaHL.

-Hyrum

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

[Mark Phippard <ma...@gmail.com>]

2008/6/30 Julian Foad <ju...@btopenworld.com>:
> 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.

I am in favor of it in concept.  I have been fortunate enough to not
run across code that does an abort/assert very often.  One example
where we did, however, was in Serf.  In Subclipse we call the info API
on a URL to see if a path exists.  When it doesn't Serf would just
abort.  This made it unusable in Subclipse because this brought down
the entire IDE.  Thankfully, Lieven eventually fixed this and a number
of other aborts in the ra_serf code.

It'd be great if whatever we did with this new change allowed the
JavaHL C++ library to turn it into a Java exception.  That might be
hoping for too much though.

-- 
Thanks

Mark Phippard
http://markphip.blogspot.com/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org