Project to review and replace C++ assert() with error() calls

Octave uses assert() calls to check certain assumptions about variables before proceeding. The problem is that an assert() statement is a land mine in the code. When a user encounters such code Octave “blows up” and immediately exits without warning and without saving user data.

Recently I replaced an input validation check in @audiorecorder which was using an assert statement (octave: ff6e74a8f7ba). Philosophically, using an assert in this instance is wrong because the consequences—instant abort and loss of data—are very much out of proportion to the severity of the problem—user mistyped an input parameter.

I think this merits a review of all of the assert() statements to see which ones are for situations where immediately stopping Octave is required, and which might be better dealt with by simply stopping current execution and issuing an error message.

There aren’t so many instances to review. I count 52 in liboctave/ and 79 in libinterp/ and none at all in src/ or libgui/. I’m uploading those lists here:

liboctave.assert.list (2.8 KB)
libinterp.assert.list (4.8 KB)

At the very least, in libinterp there is a function panic_impossible() which issues an error message and then calls abort(). For this library it would be better to replace direct calls to the C++ assert function with panic_impossible because we control the behavior of that function. For example, we could have Octave save data and do an orderly shutdown before calling abort() by coding panic_impossible in a different way. But the first task is to make sure that all instances point to this function rather than assert.

Is it the right thing to replace assert (foo) with this pattern?

if (! foo)
    panic_impossible ();

Edit: added a trial patch for the parse tree to Savannah:

Yes, that will work.

I will note that assert is a macro, defined something like this:

#ifdef NDEBUG
#  define assert(condition) ((void)0)
#else
#  define assert(condition) /*implementation defined*/
#endif

so it is a no-op if NDEBUG is defined while compiling. I would guess that this micro efficiency is irrelevant in nearly all cases in the Octave sources so always having the check is probably OK.

OTOH, maybe it would be a little nicer to write if we defined our own panic_if (COND) or panic_if_not (COND) functions? If they were defined as inline functions we could also make them turn into no-ops if compiling with NDEBUG (or some similar macro).

Yet another reason why we need a performance benchmark for Octave so we could be assure that code changes don’t impact functionality (regression test suite) nor speed (performance benchmark).

But, yes, I think the performance impact is pretty minor as well.

This would give us more flexibility. But, I don’t think we need two variants simply because it introduces more complexity and more chances to get things wrong. I would favor just PANIC_IF (cond) and if programmers need the reverse sense they can negate the conditional with !.

Where would this macro go in the source tree? error.h since functions which emit errors are probably including this already?

I would also note that liboctave doesn’t have panic_impossible and so those instances need to be reviewed separately.

@jwe: I couldn’t find the macro you quoted in the source tree. I did find something related in libgnu/assure.h but it defines macros affirm and assure rather than assert. Is it possible that we don’t have this optimization in place right now and assert is not a macro but a direct library call?

The assert macro is standard C and C++. In C, it is defined in assert.h and in C++ you should use <cassert>.

Yes, it seems like error.h would be the right thing, or maybe quit.h which is in liboctave and is included in just about everything and it also defines the base octave execution exception class.

It’s fine with me if we only define panic_if.

Since we are using C++, it should probably be and inline function instead of a macro. With that defined in a header file, we can still #define it away to a no-op if that is useful.

I’m writing further comments and proposals over on the bug report rather than here on Discourse.