Julian Foad wrote:
> This patch converts aborts into asserts where it seems most appropriate.
Brane requests that the appropriateness be carefully considered, so I here
consider whether these are checking for bugs in the software (an appropriate
use) rather than input data validation (inappropriate). For the present
purpose, I regard as a separate issue the question of whether asserts should be
enabled even in release builds.
I conclude that all of the changes in this patch are appropriate, modulo the
question of whether asserts should be enabled even in release builds. This
covers about three quarters of the abort() calls in our code; the others are
either best left alone or more difficult to evaluate.
- Julian
> Index: subversion/libsvn_fs_base/reps-strings.c
> ===================================================================
> --- subversion/libsvn_fs_base/reps-strings.c (revision 11796)
> +++ subversion/libsvn_fs_base/reps-strings.c (working copy)
> @@ -421,12 +421,13 @@
>
> /* Read in our REP. */
> SVN_ERR (svn_fs_bdb__read_rep (&rep, fs, rep_key, trail));
> + assert (rep->kind == rep_kind_delta || rep->kind == rep_kind_fulltext);
Only these two values are defined for the "kind" field, so any other value is
due to a bug.
The same change is made throughout this file.
> Index: subversion/libsvn_fs_base/trail.c
> ===================================================================
> --- subversion/libsvn_fs_base/trail.c (revision 11796)
> +++ subversion/libsvn_fs_base/trail.c (working copy)
> @@ -105,10 +107,9 @@
> trail->undo = 0;
> if (use_txn)
> {
> - /* If we're already inside a trail operation, abort() -- this is
> + /* If we're already inside a trail operation, abort -- this is
> a coding problem (and will likely hang the repository anyway). */
> - if (bfd->in_txn_trail)
> - abort();
> + assert (! bfd->in_txn_trail);
This is already documented right there as a bug detection.
> Index: subversion/libsvn_fs_base/util/skel.c
> ===================================================================
> --- subversion/libsvn_fs_base/util/skel.c (revision 11796)
> +++ subversion/libsvn_fs_base/util/skel.c (working copy)
> @@ -361,8 +362,7 @@
> int length_len;
>
> length_len = svn_fs_base__putsize (buf, sizeof (buf), skel->len);
> - if (! length_len)
> - abort ();
> + assert (length_len);
This is checking whether the value overflows its 200-byte buffer. Since the
value is passed as an apr_size_t which cannot have more than 200 digits, any
such failure must be due to a bug.
> @@ -443,8 +443,7 @@
> {
> /* If list_skel isn't even a list, somebody's not using this
> function properly. */
> - if (list_skel->is_atom)
> - abort();
> + assert (! list_skel->is_atom);
Documented right there.
(And the same again in this file.)
> Index: subversion/libsvn_fs_base/util/fs_skels.c
> ===================================================================
> --- subversion/libsvn_fs_base/util/fs_skels.c (revision 11796)
> +++ subversion/libsvn_fs_base/util/fs_skels.c (working copy)
> @@ -961,6 +962,7 @@
>
> /** Do the kind-specific stuff. **/
>
> + assert (rep->kind == rep_kind_delta || rep->kind == rep_kind_fulltext);
Same as in subversion/libsvn_fs_base/reps-strings.c, above.
> Index: subversion/libsvn_diff/token.c
> ===================================================================
> --- subversion/libsvn_diff/token.c (revision 11796)
> +++ subversion/libsvn_diff/token.c (working copy)
> @@ -76,8 +78,7 @@
> svn_diff__node_t *parent;
> int rv;
>
> - if (!token)
> - abort();
> + assert (token);
Argument validation.
> Index: subversion/libsvn_wc/entries.c
> ===================================================================
> --- subversion/libsvn_wc/entries.c (revision 11796)
> +++ subversion/libsvn_wc/entries.c (working copy)
> @@ -1026,13 +1026,11 @@
> if (strcmp (name, SVN_WC_ENTRY_THIS_DIR))
> {
> /* This is NOT the "this dir" entry */
> - if (! strcmp (name, "."))
> - {
> - /* By golly, if this isn't recognized as the "this dir"
> - entry, and it looks like '.', we're just asking for an
> - infinite recursion to happen. Abort! */
> - abort();
> - }
> +
> + /* By golly, if this isn't recognized as the "this dir"
> + entry, and it looks like '.', we're just asking for an
> + infinite recursion to happen. Abort! */
> + assert (strcmp (name, ".") != 0);
Asserting that another part of the program recognised "." as "this dir" - thus,
bug checking.
> Index: subversion/libsvn_wc/status.c
> ===================================================================
> --- subversion/libsvn_wc/status.c (revision 11796)
> +++ subversion/libsvn_wc/status.c (working copy)
> @@ -946,8 +946,7 @@
> svn_wc_status_t *parent_status;
>
> /* Don't do this. Just do NOT do this to me. */
> - if (pb && (! path))
> - abort();
> + assert (!pb || path);
Argument validation.
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 11796)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -269,8 +269,7 @@
> struct bump_dir_info *bdi;
>
> /* Don't do this. Just do NOT do this to me. */
> - if (pb && (! path))
> - abort();
> + assert (!pb || path);
Argument validation.
> @@ -547,8 +546,7 @@
> struct file_baton *f = apr_pcalloc (pool, sizeof (*f));
>
> /* I rather need this information, yes. */
> - if (! path)
> - abort();
> + assert (path);
Argument validation.
> @@ -1009,9 +1007,7 @@
>
> /* Semantic check. Either both "copyfrom" args are valid, or they're
> NULL and SVN_INVALID_REVNUM. A mixture is illegal semantics. */
> - if ((copyfrom_path && (! SVN_IS_VALID_REVNUM (copyfrom_revision)))
> - || ((! copyfrom_path) && (SVN_IS_VALID_REVNUM (copyfrom_revision))))
> - abort();
> + assert ((copyfrom_path != NULL) == SVN_IS_VALID_REVNUM (copyfrom_revision));
Argument validation.
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 11796)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -1162,8 +1162,7 @@
> target = parent_dir;
> while (strcmp (target, base_dir))
> {
> - if (target[0] == '/' && target[1] == '\0')
> - abort();
> + assert (! (target[0] == '/' && target[1] == '\0'));
Checking that the "base_dir" which was calculated to be a parent of all targets
is actually a parent of the present target. Thus, a bug check.
> Index: subversion/tests/libsvn_fs_base/skel-test.c
> ===================================================================
> --- subversion/tests/libsvn_fs_base/skel-test.c (revision 11796)
> +++ subversion/tests/libsvn_fs_base/skel-test.c (working copy)
> @@ -172,12 +173,9 @@
> static void
> put_implicit_length_byte (svn_stringbuf_t *str, char byte, char term)
> {
> - if (! skel_is_name (byte))
> - abort ();
> - if (term != '\0'
> - && ! skel_is_space (term)
> - && ! skel_is_paren (term))
> - abort ();
> + assert (skel_is_name (byte));
> + assert (term == '\0' || skel_is_space (term) || skel_is_paren (term));
> +
Argument validation.
Same throughout this file.
> Index: subversion/libsvn_repos/dump.c
> ===================================================================
> --- subversion/libsvn_repos/dump.c (revision 11796)
> +++ subversion/libsvn_repos/dump.c (working copy)
> @@ -252,8 +254,7 @@
> const char *full_path;
>
> /* A path relative to nothing? I don't think so. */
> - if (path && (! pb))
> - abort();
> + assert (!path || pb);
Argument validation.
> Index: subversion/libsvn_fs_fs/dag.c
> ===================================================================
> --- subversion/libsvn_fs_fs/dag.c (revision 11796)
> +++ subversion/libsvn_fs_fs/dag.c (working copy)
> @@ -628,10 +628,7 @@
> /* Oh, give me a clone...
> (If they're the same, we haven't cloned the transaction's root
> directory yet.) */
> - if (svn_fs_fs__id_eq (root_id, base_root_id))
> - {
> - abort ();
> - }
> + assert (! svn_fs_fs__id_eq (root_id, base_root_id));
Though I am not certain, this looks like it is detecting an error in the usage
of the function.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Nov 10 03:23:50 2004