Re: [PATCH] Replacement for "assert" in the libraries

2008/6/18 Julian Foad <julianfoad_at_btopenworld.com>:
> 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.

In general I like it. A few concerns:

> Index: subversion/include/svn_error.h
> ===================================================================
> --- subversion/include/svn_error.h (revision 31761)
> +++ subversion/include/svn_error.h (working copy)
> @@ -295,6 +295,93 @@ void svn_handle_warning(FILE *stream, sv
> err->apr_err == SVN_ERR_RA_NOT_LOCKED || \
> err->apr_err == SVN_ERR_FS_LOCK_EXPIRED)
>
> +/** Raise an error that indicates an internal malfunction.
> + *
> + * Cause the function using this macro to return an error object that
> + * describes the location of the failure, or such other behaviour as may have
> + * been specified by a call to svn_error_set_assertion_handler().

svn_error_set_malfunc_handler

> + *
> + * This macro is logically equivalent to an assertion that always fails, and
> + * should be used where the program reaches a state that cannot possibly be
> + * reached unless there is a bug in the program.
> + *
> + * @since New in 1.6.
> + */
> +#define SVN_ERR_MALFUNCTION() \
> + SVN_ERR(svn_error__internal_err(__FILE__, __LINE__, NULL))
> +
> +/** Assert that some condition is true: raise an error if it is not.
> + *
> + * If the expression @a expr is not true, cause the function using this
> + * macro to return an error object that describes the failure, or such
> + * other behaviour as may have been specified by a call to
> + * svn_error_set_assertion_handler().
> + *
> + * This should be used to check conditions that cannot possibly be false
> + * unless there is a bug in the program.
> + *
> + * @since New in 1.6.
> + */
> +#define SVN_ERR_ASSERT(expr) \
> + do { \
> + if (!(expr)) \
> + SVN_ERR(svn_error__internal_err(__FILE__, __LINE__, #expr)); \
> + } while (0)
> +
> +/** Helper function for the macros that report internal errors. */
> +svn_error_t *
> +svn_error__internal_err(const char *file, int line, const char *expr);
> +
> +/** A type of function that handles an assertion failure or other internal
> + * malfunction detected within the Subversion libraries.
> + *
> + * The error occurred in the source file @a file at line @a line, and was an
> + * assertion failure of the expression @a expr, or, if @a expr is null, an
> + * unconditional error.
> + *
> + * A function of this type must do one of:
> + * - Return an error object describing the error.
> + * - Allow the program to continue, by returning SVN_NO_ERROR.

What does that mean exactly? I mean, if I'm writing a function, and I
write an assertion, then I assume that this means that if the
assertion is false then the code's not going to keep running. I mean,
if I read:

  SVN_ERR_ASSERT(pointer != NULL);

  something(*pointer);

then I'm going to assume that, in fact, pointer is not NULL in the
second line. Or

  SVN_ERR_ASSERT(there_are_no_secrets_in(some_structure));

  send_to_some_random_person_over_the_network(some_structure);

that I'm never sharing secrets. But in your API, it's perfectly
acceptable for somebody using our libraries to implement a malfunction
handler which ignores the assertion or abort entirely? I think that
sort of makes them worthless...

(And sure, you can compile out assert()s normally too, but not
abort()s.)

> + * - Never return.
> + *
> + * The function may alter its behaviour according to compile-time
> + * and run-time and even interactive conditions.
> + *
> + * @since New in 1.6.
> + */
> +typedef svn_error_t *(*svn_error_malfunc_cb_t)
> + (const char *file, int line, const char *expr);
> +
> +/** Cause subsequent internal errors to be handled by @a func.
> + * Return the handler that was previously in effect.
> + *
> + * @a func may not be null.
> + *
> + * @note This function must be called in a single-threaded context.
> + *
> + * @since New in 1.6.
> + */
> +svn_error_malfunc_cb_t
> +svn_error_set_malfunc_handler(svn_error_malfunc_cb_t func);
> +
> +/** Handle an internal error by returning an error object describing it.
> + *
> + * This function implements @c svn_error_assertion_func_t.

svn_error_malfunc_cb_t (Brane only pointed out the one below).

> + *
> + * @since New in 1.6.
> + */
> +svn_error_t *
> +svn_error_raise_on_malfunc(const char *file, int line, const char *expr);
> +
> +/** Handle an internal error by printing a message to stderr and aborting.
> + *
> + * This function implements @c svn_error_assertion_func_t.
> + *
> + * @since New in 1.6.
> + */
> +svn_error_t *
> +svn_error_abort_on_malfunc(const char *file, int line, const char *expr);

Maybe we want to add an environment variable or something that our
command-line clients check and use this handler if it's set?

--dave

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org