Re: [Warzone-dev] assert & return macro
On Mon, Feb 02, 2009 at 08:58:11AM +0100, Per Inge Mathisen wrote: > On Sun, Feb 1, 2009 at 11:55 PM, Giel van Schijndel wrote: >>> It is misleading, though, since it can also be ASSERT_AND_RETURN or >>> just RETURN, depending on how the program is compiled and how gdb >>> reacts to abort signals. So I would prefer to keep the slight >>> ambiguity in the name. >> >> Then do I read this wrong? >>> Assert that returns given return value on failure in non-debug builds. > > In debug builds you get both an assert and a return if the condition > fails. You may not always get to the return, but it is there for > those cases where you ignore the abort signal. Still, even in non-debug builds the expression is asserted to hold true, the action taken on failure to assert is just different. For debug builds it calls some implementation defined __assert_fail-like function. On non-debug builds only an error is logged. >> I'd say that's always equal to ASSERT_OR_RETURN > > Maybe. Maybe not. But ASSERT_RETURN is what I find used elsewhere. > According to google it is somewhat frequent (26k pages), while it > gives me exactly 3 pages for either of ASSERT_OR_RETURN as > ASSERT_AND_RETURN. I'd rather base names for our functions and macros on legibility than on statistics. To me ASSERT_RETURN could mean any of ASSERT_OR_RETURN and ASSERT_AND_RETURN. -- Giel signature.asc Description: Digital signature ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] assert & return macro
On Sun, Feb 1, 2009 at 11:55 PM, Giel van Schijndel wrote: > I'd rather not use assert(false) as on some systems assert() is used to > produce a nice GUI message. E.g. on Windows you'll get a message like > "assertion $expression failed, abort, ignore, attach debugger?". > Deciding which of those buttons to click becomes difficult if you don't > get to see any sensible expression there. > > Perhaps as a fallback we could do something like this: >> #define ASSERT_XXX(expr, str_expr, ...) \ >> assert(!str_expr) > Which for ASSERT(a < b, ...) (sh|w)ould be equal to: >> assert(!"a < b"); > > That would be false as well but would still retain the benefit of single > evaluation. That sounds like a good idea. >> It is misleading, though, since it can also be ASSERT_AND_RETURN or >> just RETURN, depending on how the program is compiled and how gdb >> reacts to abort signals. So I would prefer to keep the slight >> ambiguity in the name. > > Then do I read this wrong? >> Assert that returns given return value on failure in non-debug builds. In debug builds you get both an assert and a return if the condition fails. You may not always get to the return, but it is there for those cases where you ignore the abort signal. > I'd say that's always equal to ASSERT_OR_RETURN Maybe. Maybe not. But ASSERT_RETURN is what I find used elsewhere. According to google it is somewhat frequent (26k pages), while it gives me exactly 3 pages for either of ASSERT_OR_RETURN as ASSERT_AND_RETURN. - Per ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] assert & return macro
On Sat, Jan 31, 2009 at 11:45:11PM +0100, Per Inge Mathisen wrote: > No. There are in fact three places now that the expression result is checked: > 1) Checking if we should log an error > 2) The assert() > 3) The return condition > > In my patch I only cache it in the first case for the last case, since > the second case will be void in non-debug builds anyway. But we should > consider just doing assert(false) since we log the expression and > error better in the code right above it anyway. I'd rather not use assert(false) as on some systems assert() is used to produce a nice GUI message. E.g. on Windows you'll get a message like "assertion $expression failed, abort, ignore, attach debugger?". Deciding which of those buttons to click becomes difficult if you don't get to see any sensible expression there. Perhaps as a fallback we could do something like this: > #define ASSERT_XXX(expr, str_expr, ...) \ > assert(!str_expr) Which for ASSERT(a < b, ...) (sh|w)ould be equal to: > assert(!"a < b"); That would be false as well but would still retain the benefit of single evaluation. >> Additionally I think ASSERT_OR_RETURN is a slightly better name > > It is misleading, though, since it can also be ASSERT_AND_RETURN or > just RETURN, depending on how the program is compiled and how gdb > reacts to abort signals. So I would prefer to keep the slight > ambiguity in the name. Then do I read this wrong? > Assert that returns given return value on failure in non-debug builds. I interpret that as, when I do: > ASSERT_RETURN(expr, retval, "blabla"); it'll return "retval" when "expr" yields "false" when evaluated. To me the above ASSERT_RETURN statement reads like this: * assert that "expr" holds true * failing to assert that expression, return "retval" I'd say that's always equal to ASSERT_OR_RETURN, I can't think of any case where it'd become ASSERT_AND_RETURN. Translating those two names to macros (vastly simplified and using invalid C syntax) I'd write this: > #define ASSERT_OR_RETURN(expr, retval) \ > (expr) || return(retval) > #define ASSERT_AND_RETURN(expr, retval) \ > (expr) && return(retval) I think (taking shortcut behaviour into account), the former is what you want, not the latter. -- Giel signature.asc Description: Digital signature ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] assert & return macro
On Sat, Jan 31, 2009 at 4:05 PM, Giel van Schijndel wrote: > 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"? No. There are in fact three places now that the expression result is checked: 1) Checking if we should log an error 2) The assert() 3) The return condition In my patch I only cache it in the first case for the last case, since the second case will be void in non-debug builds anyway. But we should consider just doing assert(false) since we log the expression and error better in the code right above it anyway. > Additionally, I think we shouldn't have any code at all that depends on > expressions inside of ASSERT()/assert()/etc. to have side effects. We are totally in agreement on that point. However, that is not why I do want to cache it. In some cases, quite a few actually, the expression calls a validation function that may take non-trivial amounts of time to process. I do not want us to have to worry about such issues when we write assert tests. Both with and without the patch, we check the expression twice in debug builds, and once in non-debug builds (for logging). We may want to consider making ASSERT() a noop in non-debug builds, and ASSERT_RETURN only check it once for return, to make sure we get maximum speed. > Additionally I think ASSERT_OR_RETURN is a slightly better name It is misleading, though, since it can also be ASSERT_AND_RETURN or just RETURN, depending on how the program is compiled and how gdb reacts to abort signals. So I would prefer to keep the slight ambiguity in the name. - Per ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] assert & return macro
On 1/31/09, Per Inge Mathisen wrote: > On Sat, Jan 31, 2009 at 7:43 AM, bugs buggy wrote: > > I was thinking we may do something like a ASSERT_ENTER() as well > > > How would it work? > It wouldn't work in all functions, just functions that have a known input range. Since if we get garbage input, we more or less will get garbage output, and we may assert or crash. But, if we have 'clean'(valid) input, and get garbage output, we can nail down where the values are getting clobbered. Think of the vector 3uw hack we had to do. It would just make it a bit easier to know that your routine is getting valid input. A example of this is the entering of umlauts in a input box, we don't support anything but ASCII, and so when people enter that, it passes all the routines OK, but the output isn't correct at all. ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] assert & return macro
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. -- Giel signature.asc Description: Digital signature ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] assert & return macro
On Sat, Jan 31, 2009 at 7:43 AM, bugs buggy wrote: > I was thinking we may do something like a ASSERT_ENTER() as well How would it work? - Per ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] assert & return macro
On 1/29/09, 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. > > > - Per > This could be very useful. I was thinking we may do something like a ASSERT_ENTER() as well, to catch input errors, but that would be much harder to determine a valid range for some of the input parameters. (was thinking about the signed/unsigned U/BYTE/WORD/LONG conversions as well. We still need to find those suckers) ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
[Warzone-dev] assert & return macro
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.
- Per
Index: lib/framework/debug.h
===
--- lib/framework/debug.h (revision 6540)
+++ lib/framework/debug.h (working copy)
@@ -54,6 +54,22 @@
/** Whether asserts are currently enabled. */
extern bool assertEnabled;
+/* Internal assert helper macro */
+#define ASSERT_INTERNAL(expr, expr_string, location_description, function, ...) \
+( \
+ ( \
+ (expr) ? /* if (expr) */ \
+ (void)0 \
+ : /* else */\
+ ( \
+ (void)_debug(LOG_ERROR, function, __VA_ARGS__), \
+ (void)_debug(LOG_ERROR, function, "Assert in Warzone: %s (%s), last script event: '%s'", \
+ location_description, expr_string, last_called_script_event) \
+ ) \
+ ), \
+ ( assertEnabled ? assert(expr) : (void)0 )\
+)
+
/**
* ASSERT helper macro to allow some debug functions to use an alternate
* calling location.
@@ -70,19 +86,7 @@
* so unless you have a good reason to, don't depend on it.
*/
#define ASSERT_HELPER(expr, location_description, function, ...) \
-( \
- ( \
- (expr) ? /* if (expr) */ \
- (void)0 \
- : /* else */\
- ( \
- (void)_debug(LOG_ERROR, function, __VA_ARGS__), \
- (void)_debug(LOG_ERROR, function, "Assert in Warzone: %s (%s), last script event: '%s'", \
- location_description, (#expr), last_called_script_event) \
- ) \
- ), \
- assertEnabled ? assert(expr) : (void)0 \
-)
+ ASSERT_INTERNAL(expr, #expr, location_description, function, __VA_ARGS__)
/**
*
@@ -94,7 +98,14 @@
#define ASSERT(expr, ...) \
ASSERT_HELPER(expr, AT_MACRO, __FUNCTION__, __VA_ARGS__)
+/**
+ *
+ * Assert that returns given return value on failure in non-debug builds.
+ */
+#define ASSERT_RETURN(expr, retval, ...) \
+ do { bool _wzeval = (expr); ASSERT_INTERNAL(_wzeval, #expr, AT_MACRO, __FUNCTION__, __VA_ARGS__); if (!_wzeval) return retval; } while (0)
+
/**
* Compile time assert
* Not to be used in global context!
___
Warzone-dev mailing list
Warzone-dev@gna.org
https://mail.gna.org/listinfo/warzone-dev
