On Thu, Jan 29, 2009 at 08:49:39PM +0100, Per Inge Mathisen wrote:
> I want to define a new macro ASSERT_RETURN(expr, retval, ...) that
> asserts in debug builds, and returns with the given return value in
> non-debug builds. This to make it easier to do the right thing when
> coding, which is to first check an entry condition with an assert, and
> then check it again to return on failure. It also has the added
> benefit that it will only evaluate the expression once, unlike
> ASSERT(expr); if (!expr) ... code, since even in non-debug builds we
> evaluate the expression to output logging in case of error.
> An example: ASSERT_RETURN(false, 1, "testing");
> This macro will test the expression 'false' and if it fails (always)
> it will assert, if asserts are enabled, and if not (or if continue
> from gdb is used) return 1.
> Since we have had quite a few rounds on the exact definition of the
> ASSERT macro, please have a close look at this patch.

I like the general idea. About your patch, assert() itself performs
string expansion on the given expression as well (for display purposes).
Doesn't caching the expression's result cause assert() to see another
string, i.e. just "_wzeval"?

Additionally, I think we shouldn't have any code at all that depends on
expressions inside of ASSERT()/assert()/etc. to have side effects. In
fact I think expressions that with side effects (i.e. expressions for
which multiple evaluation would be a problem) shouldn't be used at all
as part of asserting expressions.

E.g. I consider this to be bad code:
> int i = x;
> ...
> ASSERT(++i < y, "'i' out of range");

Instead something like this should be used IMO:
> int i = x;
> ...
> ++i;
> ASSERT(i < y, "'i' out of range");

So I suggest to rip the multiple evaluation-prevention code.

Additionally I think ASSERT_OR_RETURN is a slightly better name, which
IMHO captures the behaviour of that macro better. I.e. it either
successfully asserts the expression to be successful or it returns the
given value.

Other than these points I like this proposal.


Attachment: signature.asc
Description: Digital signature

Warzone-dev mailing list

Reply via email to