[Freeciv-Dev] (PR#2638) better assert checking for autogames

["Per I. Mathisen via RT" <rt@xxxxxxxxxxxxxx>]

This patch adds some better assert checking for autogames, and also
improves the logging done in get_defender() + adds an assert there.

These changes were vital for me to track down various bugs. Liberally
sprinkling CHECK_UNIT() around in a function suspected of not detecting
unit death properly is a very nice way of locating the problem.

CHECK_UNIT() is only available in games compiled with --with-debug=yes

  - Per

Index: ai/aiair.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiair.c,v
retrieving revision 1.6
diff -u -r1.6 aiair.c
--- ai/aiair.c  2002/12/22 18:14:43     1.6
+++ ai/aiair.c  2002/12/23 22:21:59
@@ -294,6 +294,8 @@
 {
   enum goto_result result = GR_FAILED;
 
+  CHECK_UNIT(punit);
+
   if (!is_airunit_refuel_point(punit->x, punit->y, 
                               pplayer, punit->type, FALSE)) {
     /* We are out in the open, what shall we do? */
Index: ai/aidiplomat.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aidiplomat.c,v
retrieving revision 1.3
diff -u -r1.3 aidiplomat.c
--- ai/aidiplomat.c     2002/12/18 17:36:18     1.3
+++ ai/aidiplomat.c     2002/12/23 22:21:59
@@ -539,7 +545,7 @@
 {
   struct city *pcity, *ctarget = NULL;
 
-  assert((pplayer != NULL) && (punit != NULL));
+  CHECK_UNIT(punit);
 
   pcity = map_get_city(punit->x, punit->y);
   generate_warmap(pcity, punit);
@@ -625,6 +631,7 @@
     ai_unit_new_role(punit, task, ctarget->x, ctarget->y);
   }
 
+  CHECK_UNIT(punit);
   assert(punit->moves_left > 0 && ctarget && punit->ai.ai_role != AIUNIT_NONE);
 
   /* GOTO unless we want to stay */
Index: ai/aitools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aitools.c,v
retrieving revision 1.68
diff -u -r1.68 aitools.c
--- ai/aitools.c        2002/12/21 11:44:00     1.68
+++ ai/aitools.c        2002/12/23 22:21:59
@@ -117,6 +117,7 @@
 **************************************************************************/
 bool ai_unit_gothere(struct unit *punit)
 {
+  CHECK_UNIT(punit);
   assert(punit->goto_dest_x != -1 && punit->goto_dest_y != -1);
   if (ai_unit_goto(punit, punit->goto_dest_x, punit->goto_dest_y)) {
     return TRUE; /* ... and survived */
@@ -137,6 +138,7 @@
   int oldx = punit->goto_dest_x, oldy = punit->goto_dest_y;
   enum unit_activity activity = punit->activity;
 
+  CHECK_UNIT(punit);
   /* TODO: log error on same_pos with punit->x|y */
   punit->goto_dest_x = x;
   punit->goto_dest_y = y;
@@ -192,6 +194,7 @@
 **************************************************************************/
 bool ai_unit_make_homecity(struct unit *punit, struct city *pcity)
 {
+  CHECK_UNIT(punit);
   if (punit->homecity == 0 && !unit_has_role(punit->type, L_EXPLORER)) {
     /* This unit doesn't pay any upkeep while it doesn't have a homecity,
      * so it would be stupid to give it one. There can also be good reasons
@@ -275,7 +278,7 @@
   struct packet_move_unit pmove;
   int sanity = punit->id;
 
-  assert(punit != NULL);
+  CHECK_UNIT(punit);
   assert(unit_owner(punit)->ai.control);
   assert(is_normal_map_pos(x, y));
   assert(is_tiles_adjacent(punit->x, punit->y, x, y));
@@ -308,7 +311,7 @@
   struct player *pplayer = unit_owner(punit);
   struct tile *ptile = map_get_tile(x,y);
 
-  assert(punit != NULL);
+  CHECK_UNIT(punit);
   assert(unit_owner(punit)->ai.control);
   assert(is_normal_map_pos(x, y));
   assert(is_tiles_adjacent(punit->x, punit->y, x, y));
Index: ai/aitools.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aitools.h,v
retrieving revision 1.31
diff -u -r1.31 aitools.h
--- ai/aitools.h        2002/12/18 19:05:21     1.31
+++ ai/aitools.h        2002/12/23 22:21:59
@@ -22,6 +22,16 @@
 struct government;
 struct player;
 
+#ifdef DEBUG
+#define CHECK_UNIT(punit)                                        \
+  assert(punit);                                                 \
+  assert(punit->type < U_LAST);                                  \
+  assert(punit->owner < MAX_NUM_PLAYERS + MAX_NUM_BARBARIANS);   \
+  assert(find_unit_by_id(punit->id));
+#else
+#define CHECK_UNIT(punit) /* check punit here when in debug mode */
+#endif
+
 enum bodyguard_enum {
   BODYGUARD_WANTED=-1,
   BODYGUARD_NONE
Index: ai/aiunit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.c,v
retrieving revision 1.241
diff -u -r1.241 aiunit.c
--- ai/aiunit.c 2002/12/21 09:39:23     1.241
+++ ai/aiunit.c 2002/12/23 22:22:00
@@ -191,7 +191,7 @@
 {
   int move_time;
   int move_rate = unit_type(punit)->move_rate;
- 
+
   switch (unit_type(punit)->move_type) {
   case LAND_MOVING:
   
@@ -242,6 +242,8 @@
 **************************************************************************/
 static bool tile_is_accessible(struct unit *punit, int x, int y)
 {
+  CHECK_UNIT(punit);
+
   if (unit_type_really_ignores_zoc(punit->type))
     return TRUE;
   if (is_my_zoc(unit_owner(punit), x, y))
@@ -274,6 +276,8 @@
   /* Range of unit's vision */
   int range;
 
+  CHECK_UNIT(punit);
+
   /* Get the range */
   /* FIXME: The vision range should NOT take into account watchtower benefit.
    * Now it is done in a wrong way: the watchtower bonus is computed locally,
@@ -619,6 +623,8 @@
   struct city *pcity = map_get_city(punit->x, punit->y);
   bool has_defense = FALSE;
 
+  CHECK_UNIT(punit);
+
   if (!pcity) {
     return FALSE;
   }
@@ -755,6 +761,8 @@
 {
   int x, y;
 
+  CHECK_UNIT(punit);
+
   if (dest) {
     x = punit->goto_dest_x;
     y = punit->goto_dest_y;
@@ -875,6 +883,8 @@
   int val = unit_belligerence_primitive(punit), cur, d;
   struct tile *ptile;
 
+  CHECK_UNIT(punit);
+
   square_iterate(pdef->x, pdef->y, 1, i, j) {
     ptile = map_get_tile(i, j);
     unit_list_iterate(ptile->units, aunit) {
@@ -921,6 +931,8 @@
   int x = punit->x, y = punit->y;
   int best = 0;
   int stack_size = unit_list_size(&(map_get_tile(punit->x, punit->y)->units));
+
+  CHECK_UNIT(punit);
   
   *dest_y = y;
   *dest_x = x;
@@ -1060,12 +1072,15 @@
   int x, y, id = punit->id;
   int count = punit->moves_left + 1; /* break any infinite loops */
 
+  CHECK_UNIT(punit);
+
   while (punit && punit->moves_left > 0 && count-- > 0
          && ai_military_findvictim(punit, &x, &y) >= threshold) {
     ai_unit_attack(punit, x, y);
     punit = find_unit_by_id(id);
   }
   assert(count >= 0);
+
   return punit;
 }
 
@@ -1079,6 +1094,8 @@
   struct city *acity = find_city_by_id(punit->ai.charge);
   int x, y, id = punit->id;
 
+  CHECK_UNIT(punit);
+
   if (aunit && aunit->owner == punit->owner) { 
     /* protect a unit */
     x = aunit->x; 
@@ -1127,6 +1144,8 @@
   int ok, best = 0;
   enum tile_terrain_type t;
 
+  CHECK_UNIT(punit);
+
   adjc_iterate(dest_x, dest_y, x1, y1) {
     ok = 0;
     t = map_get_terrain(x1, y1);
@@ -1177,6 +1196,8 @@
   int best_y = punit->y;
   int dist = 100;
   int search_dist = real_map_distance(pc->x, pc->y, punit->x, punit->y) - 1;
+
+  CHECK_UNIT(punit);
   
   square_iterate(punit->x, punit->y, search_dist, xx, yy) {
     if (map_get_continent(xx, yy) == map_get_continent(pc->x, pc->y)
@@ -1205,6 +1226,8 @@
   struct tile *ptile;
   int d_type, d_val;
 
+  CHECK_UNIT(punit);
+
   id = punit->id; x = punit->x; y = punit->y;
 
   if (is_ground_unit(punit)) boatid = find_boat(pplayer, &bx, &by, 2);
@@ -1338,7 +1361,7 @@
   }
 
   /* Dead unit shouldn't reach this point */
-  assert(player_find_unit_by_id(pplayer, id) != NULL);
+  CHECK_UNIT(punit);
   
   if (!same_pos(punit->x, punit->y, x, y)) {
     return 1;                  /* moved */
@@ -1435,6 +1458,8 @@
   int q = 0;
   struct unit_type *punittype = get_unit_type(punit->type);
 
+  CHECK_UNIT(punit);
+
 /* tired of AI abandoning its cities! -- Syela */
   if (punit->homecity != 0 && (pcity = find_city_by_id(punit->homecity))) {
     if (pcity->ai.danger != 0) { /* otherwise we can attack */
@@ -1560,6 +1585,9 @@
 static void ai_military_gohome(struct player *pplayer,struct unit *punit)
 {
   struct city *pcity;
+
+  CHECK_UNIT(punit);
+
   if (punit->homecity != 0){
     pcity=find_city_by_id(punit->homecity);
     freelog(LOG_DEBUG, "GOHOME (%d)(%d,%d)C(%d,%d)",
@@ -1915,6 +1943,8 @@
   int best = 6 * THRESHOLD + 1, cur;
   bool ground = is_ground_unit(punit);
 
+  CHECK_UNIT(punit);
+
   generate_warmap(map_get_city(punit->x, punit->y), punit);
   players_iterate(aplayer) {
     if (pplayers_allied(pplayer,aplayer)) {
@@ -1958,6 +1988,8 @@
   int ct = 10;
   struct city *pcity = NULL;
 
+  CHECK_UNIT(punit);
+
   /* Main attack loop */
   do {
     /* First find easy adjacent enemies; 2 is better than pillage */
@@ -2061,6 +2095,8 @@
   struct packet_unit_request req;
   int tradeval, best_city = -1, best=0;
 
+  CHECK_UNIT(punit);
+
   if (punit->ai.ai_role == AIUNIT_NONE) {
     if ((pcity = wonder_on_continent(pplayer, 
                                      map_get_continent(punit->x, punit->y))) 
@@ -2134,6 +2170,8 @@
   int best = 4 * punittype->move_rate, x = punit->x, y = punit->y;
   int n = 0, p = 0;
 
+  CHECK_UNIT(punit);
+
   if (!unit_list_find(&map_get_tile(punit->x, punit->y)->units, 
punit->ai.passenger))
     punit->ai.passenger = 0;
 
@@ -2210,6 +2248,7 @@
   } /* AI used to build frigates to attack and then use them as ferries -- 
Syela */
 
   /*** Find work ***/
+  CHECK_UNIT(punit);
 
   generate_warmap(map_get_city(punit->x, punit->y), punit);
   p = 0; /* yes, I know it's already zero. -- Syela */
@@ -2237,7 +2276,8 @@
     return;
   }
 
-/* do cool stuff here */
+  /* do cool stuff here */
+  CHECK_UNIT(punit);
 
   if (punit->moves_left == 0) return;
   pcity = find_city_by_id(punit->homecity);
@@ -2269,6 +2309,8 @@
   struct city *safe = NULL;
   struct unit_type *punittype = get_unit_type(punit->type);
 
+  CHECK_UNIT(punit);
+
   if (pcity) {
     /* rest in city until the hitpoints are recovered, but attempt
        to protect city from attack */
@@ -2319,6 +2361,8 @@
 {
   int id = punit->id;
 
+  CHECK_UNIT(punit);
+
   /* was getting a bad bug where a settlers caused a defender to leave home */
   /* and then all other supported units went on DEFEND_HOME/goto */
   ai_military_findjob(pplayer, punit);
@@ -2402,6 +2446,8 @@
 **************************************************************************/
 static void ai_manage_unit(struct player *pplayer, struct unit *punit)
 {
+  CHECK_UNIT(punit);
+
   /* retire useless barbarian units here, before calling the management
      function */
   if( is_barbarian(pplayer) ) {
@@ -2571,6 +2619,8 @@
   int safest = 0, safest_x = leader->x, safest_y = leader->y;
   struct unit *closest_unit = NULL;
   int dist, mindist = 10000;
+
+  CHECK_UNIT(leader);
 
   if (leader->moves_left == 0 || 
       (map_get_terrain(leader->x, leader->y) != T_OCEAN &&
Index: common/combat.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/combat.c,v
retrieving revision 1.27
diff -u -r1.27 combat.c
--- common/combat.c     2002/12/18 17:36:19     1.27
+++ common/combat.c     2002/12/23 22:22:00
@@ -503,10 +503,15 @@
   } unit_list_iterate_end;
 
   if (count > 0 && !bestdef) {
-    struct unit *debug_unit = unit_list_get(&map_get_tile(x, y)->units, 0);
-    freelog(LOG_ERROR, "Get_def bugged at (%d,%d). The most likely course"
-           " is a unit on an ocean square without a transport. The owner"
-           " of the unit is %s", x, y, unit_owner(debug_unit)->name);
+    struct tile *ptile = map_get_tile(x, y);
+    struct unit *punit = unit_list_get(&ptile->units, 0);
+
+    freelog(LOG_ERROR, "get_defender bug: %s's %s vs %s's %s (total %d"
+            " units) on %s at (%d,%d). ", unit_owner(attacker)->name,
+            unit_type(attacker)->name, unit_owner(punit)->name,
+            unit_type(punit)->name, unit_list_size(&ptile->units), 
+            get_terrain_name(ptile->terrain), x, y);
+    assert(FALSE);
   }
 
   return bestdef;