Re: assert vs. abort

[Ben Pfaff]

John Darrington <address@hidden> writes:

> I rather agree with Stroustrup when he says that the stdc assert() is
> anachronistic and too inflexible.

I don't know what exactly he means by that.  I did a web search
for "stroustrup assert" and came up with a message that describes
his comments indirectly.  I'll attempt to refute some of the
points made there in the hope that it addresses Stroustrup's
actual complaints.

$ In private email, Kevlin Henney points out that use of assert() in
$ boost headers violates the ODR (one-definition rule). Another
$ problem with assert(), of course, is that it is a macro and thus
$ doesn't respect scope rules.
$ 
$ Bjarne Stroustrup, in C++ Programming Language, 3rd, page 750, also
$ points out assert()'s lack of granularity, and further suggests it
$ should throw on failure.
$ 
$ Bjarne suggests template based alternatives.

Not all of this applies.  First, although I don't understand how
calling assert() could violate the ODR, C doesn't have the same
requirements and the problem probably doesn't apply to C.
Second, I don't think there's any way to define assert() in C
other than by macro (even if the standard didn't actually require
it to be a macro).

I'm going to assume that "lack of granularity" means that there's
no way to just enable some assertions (say, the cheapest or most
important ones).  This is true, and it's something that I've
found irksome too; witness the expensive_assert() macro in
libpspp/array.c.  The solution might be something like this:
        
http://www.fnal.gov/docs/working-groups/fpcltf/Pkg/ZMtools/doc/html/0assertN.html
i.e. different assert levels: assert(), assert1(), assert2(),
assert3().

Of course, we can't use exceptions or templates in C.

> If redefining assert() is not acceptable, then let's just write our
> own macro pspp_assert(cond) to call request_bug_report_and_abort() and
> not use <assert.h> (except perhaps in lib/* and q2c.c). 

Let's capitalize it, whatever we call it, to make it clear that
it's a macro.  (I don't like that the standard assert() is
all-lowercase.)

I would suggest just ASSERT.

> The notion (implied by the NDBUG mechanism) that assertions are
> something to be disabled in production code is a common one, but one
> that I think is misguided.  I would only set NDEBUG in code where
> speed is absolutely critical, and pspp doesn't have that criteria.

This is a common argument and a very old one; C. A. R. Hoare said
something similar, although about array bounds checks, way back
in 1972, according to a citation by Knuth.  I sympathize with the
sentiment.  But I think it's flawed.

First: Some useful assertions are very expensive.  Consider the
assertion at the end of binary_search() in array.c:
  expensive_assert (find (array, count, size, value, compare, aux) == NULL);
This assertion is invoked when the binary search didn't find a
match.  It linearly searches the array for a match.  This
assertion can fail if the array was not actually sorted or if the
comparison function is wonky.  That's something valuable to check
for, but it increases the cost of a failing binary search from
O(lg n) to O(n).  That's normally acceptable for debugging, but
not good for real use.

Also: it's impossible to tell how much assertions actually cost
you unless you occasionally turn them off and run tests with and
without them.  I've actually done this from time to time.  It's
enlightening.

Finally: some functions that benefit from assertions are
otherwise so short and simple that the assertion check dominates
their runtime.  I'm talking about something like a function that
obtains the next element in a linked list.  It might want to
verify that the list structure is intact, to guard against
accessing an element that's been removed or deallocated, but the
actual code boils down to just dereferencing a pointer.  That
might also make it yield too much code to inline the function, or
waste a lot of space.

> A pspp_assert_not_reached() macro is a good idea.

Again I prefer macros to be capitalized, for what it's worth.

> Whilst were talking about these things, I'm not too happy with the
> current implementation of request_bug_report_and_abort().  One of the
> reasons it could get called is heap exhaustion.  Any implementation of
> printf is free to use the heap, so we could end up with an infinite
> loop.  I think the strings should be statically allocated on startup,
> and just shipped to stderr using fputs.

write(STDERR_FILENO, buf, buf_len) would be even more
conservative, because it wouldn't try to obtain the lock that
POSIX requires streams to have.

How about this:

    ASSERT() -- for cheap, important checks
    ASSERT1() -- for more expensive or less important checks
    ASSERT2() -- for expensive checks
    ASSERT_LEVEL -- if defined to a number, checks at the given
    level or higher are disabled.
    NOT_REACHED() -- as discussed
-- 
Peter Seebach on managing engineers:
"It's like herding cats, only most of the engineers are already
 sick of laser pointers."