Hello,
This is a follow-up on the previous discussion about ASSERT_RETURN
that I proposed. Since then, I have been thinking, and come to some
tentative conclusions:
* Giel is right that it should be ASSERT_OR_RETURN (not
ASSERT_AND_RETURN as I mistakenly suggested)
* Both of these are too long to type and lead to unnecessary line
wrapping that are hard to read
* Adding a return value inside a macro is far too ugly. It makes the
code hard to read and make return values bug-prone to change
* assert(!expr) where expr is a const char does not have the desired
effect of displaying the contents of expr as previously suggested
I tried making a ASSERT_CONDITIONAL, which is basically an assert
inside an if expression, which meant you could write if
(!ASSERT_CONDITIONAL(something, "blah")) return 0;, but this turned
out to be exceptionally difficult, mostly because assert returns void
and cannot be placed inside a conditional expression, and it also
cannot be wrapped inside a function without losing the correct code
location number in the assert() message.
So what I came up with is the attached ASSERT_ORZ() macro. The name is
horrible, I know, but the alternative was to call it by its full
proper name ASSERT_OR_RETURN_ZERO, which nobody would bother typing
and would wrap lines like there was no tomorrow. It asserts the given
condition, and if it fails it will throw an assert and then return
zero. Zero works for the three cases where the function returns one of
zero, NULL or false in case of error, which may sound like little but
actually covers almost every case. It (and also fixed ordinary ASSERT
to work the same way) only evaluates the condition once in case of
success (for performance, not safety), and twice in case of failure in
order to give assert() the correct expression.
I made some example changes to game.c in the patch that shows how it
works, and how it saves vital typing time and code lines.
Comments are as always welcome :-)
- Per
Index: src/game.c
===================================================================
--- src/game.c (revision 6987)
+++ src/game.c (working copy)
@@ -3471,11 +3471,7 @@
UDWORD fileExtension;
DROID *psDroid, *psNext;
- ASSERT(aFileName && strlen(aFileName) > 4, "Bad savegame filename");
- if (!aFileName || strlen(aFileName) < 4)
- {
- return false;
- }
+ ASSERT_ORZ(aFileName && strlen(aFileName) > 4, "Bad savegame filename");
debug(LOG_WZ, "saveGame: %s", aFileName);
@@ -4824,14 +4820,8 @@
saveGame.GameType = saveType;
//save the current level so we can load up the STARTING point of the mission
- if (strlen(aLevelName) > MAX_LEVEL_SIZE)
- {
- ASSERT( false,
- "writeGameFile:Unable to save level name - too long (max20) - %s",
- aLevelName );
-
- return false;
- }
+ ASSERT_ORZ(strlen(aLevelName) < MAX_LEVEL_SIZE, "Unable to save level name - too long (max %d) - %s",
+ (int)MAX_LEVEL_SIZE, aLevelName);
sstrcpy(saveGame.levelName, aLevelName);
//save out the players power
@@ -5233,11 +5223,7 @@
for (i=0; i < psSaveDroid->numWeaps; i++)
{
int weapon = getCompFromName(COMP_WEAPON, psSaveDroid->asWeaps[i].name);
- if( weapon < 0)
- {
- ASSERT(false, "This component does not exist : %s", psSaveDroid->asWeaps[i].name );
- return NULL;
- }
+ ASSERT_ORZ(weapon >= 0, "This component does not exist : %s", psSaveDroid->asWeaps[i].name);
psTemplate->asWeaps[i] = weapon;
}
@@ -5333,12 +5319,7 @@
}
psTemplate->asParts[i] = (UDWORD)compInc;
}
- if (!found)
- {
- //ignore this record
- ASSERT( found,"buildUnitFromSavedUnit; failed to find weapon" );
- return NULL;
- }
+ ASSERT_ORZ(found, "Failed to find weapon");
psTemplate->numWeaps = psSaveDroid->numWeaps;
if (psSaveDroid->numWeaps > 0)
@@ -5346,11 +5327,7 @@
for(i = 0;i < psTemplate->numWeaps;i++)
{
int weapon = getCompFromName(COMP_WEAPON, psSaveDroid->asWeaps[i].name);
- if( weapon < 0)
- {
- ASSERT(false, "This component does not exist : %s", psSaveDroid->asWeaps[i].name );
- return NULL;
- }
+ ASSERT_ORZ(weapon >= 0, "This component does not exist : %s", psSaveDroid->asWeaps[i].name);
psTemplate->asWeaps[i] = weapon;
}
}
@@ -5382,13 +5359,8 @@
psSaveDroid->player, false);
}
- if(psDroid == NULL)
- {
- ASSERT( false,"buildUnitFromSavedUnit; failed to build unit" );
- return NULL;
- }
+ ASSERT_ORZ(psDroid, "Failed to build unit");
-
//copy the droid's weapon stats
for (i=0; i < psDroid->numWeaps; i++)
{
@@ -5744,23 +5716,14 @@
}
psTemplate->asParts[i] = (UDWORD)compInc;
}
- if (!found)
- {
- //ignore this record
- ASSERT( found,"buildUnitFromSavedUnit; failed to find weapon" );
- return NULL;
- }
+ ASSERT_ORZ(found, "Failed to find weapon");
psTemplate->numWeaps = psSaveDroid->numWeaps;
if (psSaveDroid->numWeaps > 0)
{
for(i = 0;i < psTemplate->numWeaps;i++)
{
int weapon = getCompFromName(COMP_WEAPON, psSaveDroid->asWeaps[i].name);
- if( weapon < 0)
- {
- ASSERT(false, "This component does not exist : %s", psSaveDroid->asWeaps[i].name );
- return NULL;
- }
+ ASSERT_ORZ(weapon >= 0, "This component does not exist : %s", psSaveDroid->asWeaps[i].name);
psTemplate->asWeaps[i] = weapon;
}
}
@@ -5794,11 +5757,7 @@
psSaveDroid->player, false);
}
- if(psDroid == NULL)
- {
- ASSERT( false,"buildUnitFromSavedUnit; failed to build unit" );
- return NULL;
- }
+ ASSERT_ORZ(psDroid, "Failed to build unit");
turnOffMultiMsg(false);
@@ -6997,11 +6956,7 @@
psStructure = buildStructure(psStats, psSaveStructure->x, psSaveStructure->y,
psSaveStructure->player,true);
- if (!psStructure)
- {
- ASSERT( false, "loadSaveStructure:Unable to create structure" );
- return false;
- }
+ ASSERT_ORZ(psStructure, "Unable to create structure");
/*The original code here didn't work and so the scriptwriters worked
round it by using the module ID - so making it work now will screw up
@@ -7284,13 +7239,8 @@
continue;
}
- psStructure = buildStructure(psStats, psSaveStructure->x, psSaveStructure->y,
- psSaveStructure->player,true);
- if (!psStructure)
- {
- ASSERT( false, "loadSaveStructure:Unable to create structure" );
- return false;
- }
+ psStructure = buildStructure(psStats, psSaveStructure->x, psSaveStructure->y, psSaveStructure->player,true);
+ ASSERT_ORZ(psStructure, "Unable to create structure");
/*The original code here didn't work and so the scriptwriters worked
round it by using the module ID - so making it work now will screw up
@@ -7724,13 +7674,8 @@
continue;
}
- psStructure = buildStructure(psStats, psSaveStructure->x, psSaveStructure->y,
- psSaveStructure->player,true);
- if (!psStructure)
- {
- ASSERT( false, "loadSaveStructure:Unable to create structure" );
- return false;
- }
+ psStructure = buildStructure(psStats, psSaveStructure->x, psSaveStructure->y, psSaveStructure->player, true);
+ ASSERT_ORZ(psStructure, "Unable to create structure");
/*The original code here didn't work and so the scriptwriters worked
round it by using the module ID - so making it work now will screw up
@@ -8541,11 +8486,7 @@
pFeature = buildFeature(psStats, psSaveFeature->x, psSaveFeature->y,true);
//will be added to the top of the linked list
//pFeature = apsFeatureLists[0];
- if (!pFeature)
- {
- ASSERT( false, "loadSaveFeature:Unable to create feature" );
- return false;
- }
+ ASSERT_ORZ(pFeature, "Unable to create feature");
//restore values
pFeature->id = psSaveFeature->id;
pFeature->direction = psSaveFeature->direction;
@@ -8645,11 +8586,7 @@
pFeature = buildFeature(psStats, psSaveFeature->x, psSaveFeature->y,true);
//will be added to the top of the linked list
//pFeature = apsFeatureLists[0];
- if (!pFeature)
- {
- ASSERT( false, "loadSaveFeature:Unable to create feature" );
- return false;
- }
+ ASSERT_ORZ(pFeature, "Unable to create feature");
//restore values
pFeature->id = psSaveFeature->id;
pFeature->direction = psSaveFeature->direction;
Index: lib/framework/debug.h
===================================================================
--- lib/framework/debug.h (revision 6987)
+++ lib/framework/debug.h (working copy)
@@ -54,9 +54,19 @@
/** Whether asserts are currently enabled. */
extern bool assertEnabled;
+/** Deals with failure in an assert. Expression is (re-)evaluated for output in the assert() call. */
+#define ASSERT_FAILURE(expr, expr_string, location_description, function, ...) \
+ ( \
+ (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.
+ * Internal assert helper macro to allow some debug functions to use an alternate calling location.
+ * Expression is only evaluated once if true, if false it is evaluated another time to provide decent
+ * feedback on OSes that have good GUI facilities for asserts and lousy backtrace facilities.
*
* \param expr Expression to assert on.
* \param location_description A string describing the calling location, e.g.:
@@ -71,17 +81,10 @@
*/
#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 \
+ (expr) ? /* if (expr) */ \
+ (void)0 \
+ : /* else */\
+ ASSERT_FAILURE(expr, #expr, location_description, function, __VA_ARGS__) \
)
/**
@@ -94,7 +97,15 @@
#define ASSERT(expr, ...) \
ASSERT_HELPER(expr, AT_MACRO, __FUNCTION__, __VA_ARGS__)
+/**
+ *
+ * Assert-or-return-zero, macro that returns zero (can also be interpreted as false or NULL) on failure,
+ * and also provides asserts and debug output for debugging.
+ */
+#define ASSERT_ORZ(expr, ...) \
+ do { bool _wzeval = (expr); if (!_wzeval) { ASSERT_FAILURE(expr, #expr, AT_MACRO, __FUNCTION__, __VA_ARGS__); return 0; } } 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
