Function call inside assert == bad?

Question

I have just dug up a bug in some code we're working with* that was failing due to the following situation:

Assert(SomeVitalFunction(foo) == OK)

This worked fine all the time the DEBUG macros were #defined:

#ifdef DEBUG
 #define Assert(x) if((x) == 0){/*some error handling*/}
#else
 #define Assert(x)
#endif

But when we #undef'd DEBUG it has the effect of deleting the vital function call from the code.

I can't for the life of me work out how that could ever work with DEBUG #undef'd, and it seems a bad idea generally to put any sort of function call inside an assert like this.

Have I missed something?

* = Edit to clarify following Carpetsmoker's comment: The code comes from a particularly backward cabal of Elbonian code slaves, our task has been to hack, slash, shave, polish, sanitize and apply lipstick to the thing.

• Martin Tournoij: The point of assert is to "help programmers find bugs in their programs" ... So all aserts should ideally be removable, and everything should still work the same. So you are right in your observation, and you or one of your coworkers made a mistake in the past.
• AAT: I wrote a Visual Studio macro long ago to look through the codebase for this (in those days, looking for ASSERT(...(...) - i.e. 2 or more opening brackets before the closing one) because it is such a huge problem when it happens.

Answer 1

You have missed nothing.

Asserts should always be written as if they could disappear at the flick of a compiler switch.

You can call functions that take a relatively long time to complete inside an assert (for example analysing the integrity of a data structure), because the function call will not be present in the release build. The flip side of this is that you cannot call functions that are necessary for correct operation.

Answer 2

It depends upon what SomeVitalFunction is doing. If it has no interesting side-effects, it is ok to use it inside an assert. But if calling or not calling SomeVitalFunction is essential to the program, it is a bug.

For instance, on POSIX, kill(2) with a 0 signal is only useful to test if a process is living. I would imagine that you might be sometimes tempted to use

 assert(kill(sompid, 0) == 0); // process sompid exists

assuming that you always suppose that the process sompid is still running.

Likewise, you might use assert(hash_table_count(htbl)>0); to check that some hash table htbl is not empty.

BTW, notice that assert(3) is documented as being ignored if you compile with -DNDEBUG preprocessor option (not if -DDEBUG is not given).