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

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

Raimar Falke wrote:
> 
> On Sun, Oct 28, 2001 at 02:58:20PM -0500, Jason Dorje Short wrote:
> > Raimar Falke wrote:
> > >
> > > On Fri, Oct 26, 2001 at 01:10:17AM -0700, jdorje@xxxxxxxxxxxxxxxxxxxxx 
> > > wrote:
> > > > The attached patch implements and uses a macro called check_map_pos(&x,
> > > > &y) to check the coordinates (x,y) for normalness.
> >
> > > I think that we agreed that instead of replacing normalize_map_pos,
> > > is_real_tile and co with check_map_pos we just remove them and trust
> > > that we would catch all bad position in an access method (one which
> > > uses map_inx). Look at this problem from this point of view: now every
> > > method only accepts normal map positions. Why should some method have
> > > a check for this and some not. Because of historical reasons?
> > > No. Either all method have such a check or none. And I agree that none
> > > is ok. If there are problems we can add extra checks. But we don't
> > > expect problems ;)
> >
> > Yes, but I thought we were going to go with all-out replacement first
> > and then scale back.
> >
> > No matter, I can remove the spurious checks.  I'd prefer to only remove
> > the obviously spurious ones, though, and leave any that might be
> > debatable until it's been in CVS for a while.
> 
> We will see how many checks are in your next patch.

I'm strongly opposed to the just-one-check-in-map-inx method of doing
this.  If there is a bug, we will never find it.  Having too many
check_map_pos calls is minor compared to this.

This patch has removed some of the check_map_pos calls (as many as I
felt safe removing).

> > There really hasn't been enough testing of this to be even
> > reasonably sure things are correct otherwise, and if we don't catch
> > any mistakes in check_map_pos the odds are we won't catch them.
> 
> Note that before I accept this patch some extra checks have to be
> inserted into map_inx.

IMO check_map_pos is check_map_pos is check_map_pos - the problems with
having a failure at one place are the same as having them at another. 
map_inx is one place where a check needs to be done, but I don't think
it's more special than the others.  What extra check would you want?

jason

? rc
? topology-current
? old
? topology
? tilespec_patch
? corecleanup_08.ReadMe
? civserver-regular
Index: client/climisc.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/climisc.c,v
retrieving revision 1.63
diff -u -r1.63 climisc.c
--- client/climisc.c    2001/10/26 08:07:09     1.63
+++ client/climisc.c    2001/10/29 00:53:50
@@ -477,10 +477,9 @@
       player_in_city_radius(game.player_ptr, x1, y1);
   int pos2_is_in_city_radius = 0;
 
-  assert(is_real_tile(x1, y1));
+  check_map_pos(&x1, &y1);
 
   if (is_real_tile(x2, y2)) {
-    normalize_map_pos(&x2, &y2);
     assert(is_tiles_adjacent(x1, y1, x2, y2));
 
     if (!map_get_tile(x2, y2)->known) {
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/29 00:53:51
@@ -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/packhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
retrieving revision 1.196
diff -u -r1.196 packhand.c
--- client/packhand.c   2001/10/18 16:45:31     1.196
+++ client/packhand.c   2001/10/29 00:53:52
@@ -437,7 +437,7 @@
 static void handle_city_packet_common(struct city *pcity, int is_new,
                                       int popup, int investigate)
 {
-  int i;
+  int i, is_real;
 
   if(is_new) {
     unit_list_init(&pcity->units_supported);
@@ -463,7 +463,8 @@
     int d = (2 * r) + 1;
     int x = pcity->x - r;
     int y = pcity->y - r;
-    normalize_map_pos(&x, &y);
+    is_real = normalize_map_pos(&x, &y);
+    assert(is_real);
     update_map_canvas(x, y, d, d, 1);
   } else {
     refresh_tile_mapcanvas(pcity->x, pcity->y, 1);
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/29 00:53:53
@@ -1114,7 +1114,6 @@
 
   *solid_bg = 0;
 
-  assert(is_normal_map_pos(x, y));
   if (!tile_is_known(x, y))
     return -1;
 
@@ -1341,7 +1340,6 @@
   *solid_bg = 0;
   *pplayer = NULL;
 
-  assert(is_normal_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: client/gui-gtk/mapview.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/mapview.c,v
retrieving revision 1.106
diff -u -r1.106 mapview.c
--- client/gui-gtk/mapview.c    2001/10/26 08:07:13     1.106
+++ client/gui-gtk/mapview.c    2001/10/29 00:53:55
@@ -2225,10 +2225,6 @@
     return;
   }
 
-  /* Replace with check for is_normal_tile later */
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
-
   fog = tile_is_known(x, y) == TILE_KNOWN_FOGGED && draw_fog_of_war;
   pcity = map_get_city(x, y);
   punit = get_drawable_unit(x, y, citymode);
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/29 00:53:56
@@ -1068,9 +1068,6 @@
   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);
-
   tile0 = map_get_tile(x, y);
   debug_log_move_costs("Resetting move costs for", x, y, tile0);
 
@@ -1128,6 +1125,7 @@
 ***************************************************************/
 int is_tiles_adjacent(int x0, int y0, int x1, int y1)
 {
+  check_map_pos(&x1, &y1);
   adjc_iterate(x0, y0, x, y) {
     if (x == x1 && y == y1)
       return 1;
@@ -1156,10 +1154,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 +1162,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 +1170,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 +1179,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 +1187,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 +1195,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 +1203,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 +1214,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 +1225,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 +1234,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 +1243,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 +1252,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 +1392,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/29 00:53:56
@@ -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)],         \
@@ -279,12 +281,19 @@
  * not normal/proper.
  */
 #define IS_BORDER_MAP_POS(x, y)              \
-  ((y) == 0 || (x) == 0 ||                   \
+  (check_map_pos(&x, &y),                    \
+   (y) == 0 || (x) == 0 ||                   \
    (y) == map.ysize-1 || (x) == map.xsize-1)
 
 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 +461,6 @@
   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));                  \
   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/29 00:53:57
@@ -771,12 +771,9 @@
 ***************************************************************/
 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;
 }
@@ -955,10 +952,6 @@
 struct player_tile *map_get_player_tile(int x, int y,
                                        struct player *pplayer)
 {
-  if (!is_real_tile(x, y)) {
-    freelog(LOG_ERROR, "Trying to get nonexistant tile at %i,%i", x, y);
-  }
-  nearest_real_pos(&x, &y);
   return pplayer->private_map + map_inx(x, y);
 }
  
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/29 00:53:57
@@ -128,7 +128,6 @@
        assert(unit_owner(punit) == pplayer);
       } unit_list_iterate_end;
 
-      assert(is_normal_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/29 00:53:58
@@ -405,8 +405,6 @@
 **************************************************************************/
 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);
   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 +645,6 @@
   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;
 
   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/29 00:54:01
@@ -1628,8 +1628,6 @@
   idex_register_unit(punit);
   punit->owner=pplayer->player_no;
 
-  assert(is_real_tile(x, y));
-  normalize_map_pos(&x, &y);
   punit->x = x;
   punit->y = y;
 
@@ -2894,8 +2892,6 @@
   struct player *pplayer = get_player(playerid);
   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));
 
   conn_list_do_buffer(&pplayer->connections);