[Freeciv-Dev] [PATCH] check_map_pos change (PR#1031)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

The attached patch implements and uses a macro called check_map_pos(&x,
&y) to check the coordinates (x,y) for normalness.

The idea, of course, is to avoid expensively calling normalize_map_pos
at the start of every function, as is done now.  This patch replaces
many such checks with a check_map_pos call.  The purpose of this call is
three-fold: to catch any non-normal positions being handed around (which
would be a bug), to optionally fix these positions so as to avoid a
crash, and to be as fast as possible.

The current #definition of check_map_pos is

#define check_map_pos(x,y) assert(is_normal_map_pos(&x, &y))

which accomplishes goals #1 and #3.  It's a bit clunky, but should be
good to start out with.

Not all normalize_map_pos calls need to be replaced by check_map_pos. 
Ultimately, only those that do direct calculations with the coordinates
need check_map_pos; those that just pass the coordinates on to another
function do not.  To start with, it's probably safest to put
check_map_pos in place of every normalize_map_pos and then scale back. 
The only time I've broken this rule is with the accessor functions in
map.c which clearly call MAP_TILE/map_inx immediately.

Finally, I have not replaced all normalize_map_pos invocations.  Of
course, some of them are necessary but there are a lot that are dubious
(especially in the different GUI's of the client).  To make things
simple, I've only made replacements I'm pretty sure of.

Everything seems to continue to run fine under this patch.  In debug
mode, there should be no savings, but if you compile with NDEBUG things
should be significantly faster (I haven't profiled this).  Things could
be sped up substantially in DEBUG mode by having is_normal_map_pos not
call normalize_map_pos, if desired.

jason

? rc
? topology-rfc.txt
? old
? topology
? tilespec_patch
? corecleanup_08.ReadMe
? topology.c
? civserver-regular
? a.out
? client/mapview_common.c
? client/mapview_common.h
Index: client/goto.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/goto.c,v
retrieving revision 1.27
diff -u -r1.27 goto.c
--- client/goto.c       2001/10/08 12:11:16     1.27
+++ client/goto.c       2001/10/26 07:58:53
@@ -618,9 +618,7 @@
 {
   int x1, y1, is_real;
 
-  /* Replace with check for is_normal_tile later */  
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
+  check_map_pos(&x, &y);
 
   is_real = MAPSTEP(x1, y1, x, y, dir);
 
@@ -797,9 +795,7 @@
 
   assert(is_active);
 
-  /* Replace with check for is_normal_tile later */
-  assert(is_real_tile(dest_x, dest_y));
-  normalize_map_pos(&dest_x, &dest_y);
+  check_map_pos(&dest_x, &dest_y);
 
   if (!goto_map.vector[dest_x][dest_y]) {
     undraw_line();
Index: client/tilespec.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/tilespec.c,v
retrieving revision 1.56
diff -u -r1.56 tilespec.c
--- client/tilespec.c   2001/10/26 07:24:55     1.56
+++ client/tilespec.c   2001/10/26 07:58:56
@@ -1114,7 +1114,7 @@
 
   *solid_bg = 0;
 
-  assert(is_normal_map_pos(x, y));
+  check_map_pos(&x, &y);
   if (!tile_is_known(x, y))
     return -1;
 
@@ -1341,7 +1341,7 @@
   *solid_bg = 0;
   *pplayer = NULL;
 
-  assert(is_normal_map_pos(abs_x0, abs_y0));
+  check_map_pos(&abs_x0, &abs_y0);
   ptile=map_get_tile(abs_x0, abs_y0);
 
   if (tile_is_known(abs_x0,abs_y0) == TILE_UNKNOWN) {
Index: common/map.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
retrieving revision 1.97
diff -u -r1.97 map.c
--- common/map.c        2001/10/15 13:42:50     1.97
+++ common/map.c        2001/10/26 07:58:58
@@ -1068,8 +1068,7 @@
   int maxcost = 72; /* should be big enough without being TOO big */
   struct tile *tile0, *tile1;
 
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
+  check_map_pos(&x, &y);
 
   tile0 = map_get_tile(x, y);
   debug_log_move_costs("Resetting move costs for", x, y, tile0);
@@ -1156,10 +1155,6 @@
 ***************************************************************/
 struct tile *map_get_tile(int x, int y)
 {
-  int is_real = normalize_map_pos(&x, &y);
-
-  assert(is_real);
-
   return MAP_TILE(x, y);
 }
 
@@ -1168,10 +1163,7 @@
 ***************************************************************/
 signed short map_get_continent(int x, int y)
 {
-  if (!normalize_map_pos(&x, &y))
-    return -1;
-  else
-    return MAP_TILE(x, y)->continent;
+  return MAP_TILE(x, y)->continent;
 }
 
 /***************************************************************
@@ -1179,8 +1171,6 @@
 ***************************************************************/
 void map_set_continent(int x, int y, int val)
 {
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
   MAP_TILE(x, y)->continent = val;
 }
 
@@ -1190,10 +1180,7 @@
 ***************************************************************/
 enum tile_terrain_type map_get_terrain(int x, int y)
 {
-  if (!normalize_map_pos(&x, &y))
-    return T_UNKNOWN;
-  else
-    return MAP_TILE(x, y)->terrain;
+  return MAP_TILE(x, y)->terrain;
 }
 
 /***************************************************************
@@ -1201,10 +1188,7 @@
 ***************************************************************/
 enum tile_special_type map_get_special(int x, int y)
 {
-  if (!normalize_map_pos(&x, &y))
-    return S_NO_SPECIAL;
-  else
-    return MAP_TILE(x, y)->special;
+  return MAP_TILE(x, y)->special;
 }
 
 /***************************************************************
@@ -1212,8 +1196,6 @@
 ***************************************************************/
 void map_set_terrain(int x, int y, enum tile_terrain_type ter)
 {
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
   MAP_TILE(x, y)->terrain = ter;
 }
 
@@ -1222,9 +1204,6 @@
 ***************************************************************/
 void map_set_special(int x, int y, enum tile_special_type spe)
 {
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
-
   MAP_TILE(x, y)->special |= spe;
 
   if (spe & (S_ROAD | S_RAILROAD))
@@ -1236,8 +1215,6 @@
 ***************************************************************/
 void map_clear_special(int x, int y, enum tile_special_type spe)
 {
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
   MAP_TILE(x, y)->special &= ~spe;
 
   if (spe & (S_ROAD | S_RAILROAD))
@@ -1249,8 +1226,6 @@
 ***************************************************************/
 struct city *map_get_city(int x, int y)
 {
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
   return MAP_TILE(x, y)->city;
 }
 
@@ -1260,8 +1235,6 @@
 ***************************************************************/
 void map_set_city(int x, int y, struct city *pcity)
 {
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
   MAP_TILE(x, y)->city = pcity;
 }
 
@@ -1271,10 +1244,7 @@
 ***************************************************************/
 enum known_type tile_is_known(int x, int y)
 {
-  if (!normalize_map_pos(&x, &y))
-    return TILE_UNKNOWN;
-  else
-    return (enum known_type) (MAP_TILE(x, y)->known);
+  return (enum known_type) (MAP_TILE(x, y)->known);
 }
 
 /***************************************************************
@@ -1283,9 +1253,8 @@
 ***************************************************************/
 int same_pos(int x1, int y1, int x2, int y2)
 {
-  assert(is_real_tile(x1, y1) && is_real_tile(x2, y2));
-  normalize_map_pos(&x1, &y1);
-  normalize_map_pos(&x2, &y2);
+  check_map_pos(&x1, &y1);
+  check_map_pos(&x2, &y2);
   return (x1 == x2 && y1 == y2);
 }
 
@@ -1424,9 +1393,8 @@
 **************************************************************************/
 int is_move_cardinal(int start_x, int start_y, int end_x, int end_y)
 {
-  assert(is_real_tile(start_x, start_y) && is_real_tile(end_x, end_y));
-  normalize_map_pos(&start_x, &start_y);
-  normalize_map_pos(&end_x, &end_y);
+  check_map_pos(&start_x, &start_y);
+  check_map_pos(&end_x, &end_y);
   assert(is_tiles_adjacent(start_x, start_y, end_x, end_y));
 
   /* FIXME: this check will not work with an orthogonal map */
Index: common/map.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
retrieving revision 1.100
diff -u -r1.100 map.h
--- common/map.h        2001/10/19 08:12:52     1.100
+++ common/map.h        2001/10/26 07:59:00
@@ -13,6 +13,8 @@
 #ifndef FC__MAP_H
 #define FC__MAP_H
 
+#include <assert.h>
+
 #include "player.h"
 #include "terrain.h"
 #include "unit.h"
@@ -208,7 +210,7 @@
   (((Y)<0) ? 0 : (((Y)>=map.ysize) ? map.ysize-1 : (Y)))
 
 #define map_inx(x,y) \
-  ((x)+(y)*map.xsize)
+  (check_map_pos(&x, &y), (x)+(y)*map.xsize)
 
 #define DIRSTEP(dest_x, dest_y, dir)   \
 (    (dest_x) = DIR_DX[(dir)],         \
@@ -285,6 +287,12 @@
 int normalize_map_pos(int *x, int *y);
 void nearest_real_pos(int *x, int *y);
 
+/*
+ * Verifies that the position is normal, possibly doing some corrections
+ * if it is not.
+ */
+#define check_map_pos(x, y) assert(is_normal_map_pos(*(x), *(y)))
+
 void rand_neighbour(int x0, int y0, int *x, int *y);
 
 int is_water_adjacent_to_tile(int x, int y);
@@ -452,7 +460,7 @@
   int x_itr, y_itr, dir_itr, MACRO_border;                                    \
   int MACRO_center_x = (center_x);                                            \
   int MACRO_center_y = (center_y);                                            \
-  assert(is_normal_map_pos(MACRO_center_x, MACRO_center_y));                  \
+  check_map_pos(&MACRO_center_x, &MACRO_center_y);                            \
   MACRO_border = IS_BORDER_MAP_POS(MACRO_center_x, MACRO_center_y);           \
   for (dir_itr = 0; dir_itr < 8; dir_itr++) {                                 \
     DIRSTEP(x_itr, y_itr, dir_itr);                                           \
Index: server/maphand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/maphand.c,v
retrieving revision 1.87
diff -u -r1.87 maphand.c
--- server/maphand.c    2001/10/11 12:37:06     1.87
+++ server/maphand.c    2001/10/26 07:59:02
@@ -771,11 +771,8 @@
 ***************************************************************/
 int map_get_known_and_seen(int x, int y, struct player *pplayer)
 {
-  int is_real = normalize_map_pos(&x, &y);
   int playerid=pplayer->player_no;
   int offset = map_inx(x, y);
-
-  assert(is_real);
 
   return ((map.tiles + offset)->known) & (1u << playerid) &&
       (pplayer->private_map + offset)->seen;
Index: server/sanitycheck.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/sanitycheck.c,v
retrieving revision 1.13
diff -u -r1.13 sanitycheck.c
--- server/sanitycheck.c        2001/10/14 21:02:17     1.13
+++ server/sanitycheck.c        2001/10/26 07:59:02
@@ -128,7 +128,7 @@
        assert(unit_owner(punit) == pplayer);
       } unit_list_iterate_end;
 
-      assert(is_normal_map_pos(pcity->x, pcity->y));
+      check_map_pos(&pcity->x, &pcity->y);
       assert(map_get_terrain(pcity->x, pcity->y) != T_OCEAN);
 
       city_map_iterate(x, y) {
Index: server/settlers.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/settlers.c,v
retrieving revision 1.112
diff -u -r1.112 settlers.c
--- server/settlers.c   2001/10/14 21:02:17     1.112
+++ server/settlers.c   2001/10/26 07:59:05
@@ -405,8 +405,7 @@
 **************************************************************************/
 static int is_already_assigned(struct unit *myunit, struct player *pplayer, 
int x, int y)
 {
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
+  check_map_pos(&x, &y);
   if (same_pos(myunit->x, myunit->y, x, y) ||
       same_pos(myunit->goto_dest_x, myunit->goto_dest_y, x, y)) {
 /* I'm still not sure this is exactly right -- Syela */
@@ -647,8 +646,8 @@
   int ii[12] = { -1, 0, 1, -1, 1, -1, 0, 1, 0, -2, 2, 0 };
   int jj[12] = { -1, -1, -1, 0, 0, 1, 1, 1, -2, 0, 0, 2 };
   struct tile *ptile;
-  if (!normalize_map_pos(&x, &y))
-    return 0;
+
+  check_map_pos(&x, &y);
 
   for (k = 0; k < 12; k++) {
     int x1 = x + ii[k], y1 = y + jj[k];
Index: server/unittools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
retrieving revision 1.143
diff -u -r1.143 unittools.c
--- server/unittools.c  2001/10/15 13:42:52     1.143
+++ server/unittools.c  2001/10/26 07:59:10
@@ -1628,8 +1628,7 @@
   idex_register_unit(punit);
   punit->owner=pplayer->player_no;
 
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
+  check_map_pos(&x, &y);
   punit->x = x;
   punit->y = y;
 
@@ -2895,7 +2894,7 @@
   struct tile *psrctile = map_get_tile(src_x, src_y);
   struct tile *pdesttile = map_get_tile(dest_x, dest_y);
 
-  assert(is_normal_map_pos(dest_x, dest_y));
+  check_map_pos(&dest_x, &dest_y);
 
   conn_list_do_buffer(&pplayer->connections);