[Freeciv-Dev] Re: Unsafe assertions. (PR#864)

[Thue Janus Kristensen <thue@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Jul 27, 2001 at 11:24:58AM -0700, Trent Piepho wrote:
> On Fri, 27 Jul 2001, Thue Janus Kristensen wrote:
> > Here is a patch:
> 
> I think Jason's way was a lot better.  You have weakened all the assertions by
> only checking is_real instead of checking for is_normal.  You also normalize
> every time, when the author might very well have intended to assert that
> normalization was NOT necessary.

Note that I did not weaken the assertions. normalize_map_pos() returns TRUE
even if the tile is not normalized.

Checking again I agree that there is 3 places where a normalization should not
be neccesary, and we could (remove a fluff normalize_map_pos)/(make the
assertion is_normal_tile instead of is_real_tile). I don't want to make the
assertions stronger or remove normalizations here just before the 1.12.0 
release,
but I have inserted comments that it should be done.

-Thue

diff -Nur -X/mnt/data/freeciv-dev/freeciv/diff_ignore freeciv/client/goto.c 
codeciv/client/goto.c
--- freeciv/client/goto.c       Fri Jul 27 10:20:01 2001
+++ codeciv/client/goto.c       Fri Jul 27 19:11:28 2001
@@ -618,12 +618,15 @@
 {
   int x1, y1;
 
-  assert(normalize_map_pos(&x, &y));
+  /* Replace with check for is_normal_tile later */  
+  assert(is_real_tile(x, y));
+  normalize_map_pos(&x, &y);
 
   x1 = x + DIR_DX[dir];
   y1 = y + DIR_DY[dir];
   /* It makes no sense to draw a goto line to a non-existant tile. */
-  assert(normalize_map_pos(&x1, &y1));
+  assert(is_real_tile(x1, y1));
+  normalize_map_pos(&x1, &y1);
 
   if (dir >= 4) {
     x = x1;
@@ -776,7 +779,8 @@
     if (goto_map.vector[x][y] & (1<<dir)) {
       int new_x = x + DIR_DX[dir];
       int new_y = y + DIR_DY[dir];
-      assert(normalize_map_pos(&new_x, &new_y));
+      assert(is_real_tile(new_x, new_y));
+      normalize_map_pos(&new_x, &new_y);
 
       /* expand array as neccesary */
       if (route_index == route_length) {
@@ -806,7 +810,10 @@
   int start_index;
 
   assert(is_active);
-  assert(normalize_map_pos(&dest_x, &dest_y));
+
+  /* Replace with check for is_normal_tile later */
+  assert(is_real_tile(dest_x, dest_y));
+  normalize_map_pos(&dest_x, &dest_y);
 
   if (!goto_map.vector[dest_x][dest_y]) {
     undraw_line();
diff -Nur -X/mnt/data/freeciv-dev/freeciv/diff_ignore 
freeciv/client/gui-gtk/mapview.c codeciv/client/gui-gtk/mapview.c
--- freeciv/client/gui-gtk/mapview.c    Fri Jul 27 10:20:02 2001
+++ codeciv/client/gui-gtk/mapview.c    Fri Jul 27 19:13:18 2001
@@ -1950,7 +1950,8 @@
 
   dest_x = src_x + DIR_DX[dir];
   dest_y = src_y + DIR_DY[dir];
-  assert(normalize_map_pos(&dest_x, &dest_y));
+  assert(is_real_tile(dest_x, dest_y));
+  normalize_map_pos(&dest_x, &dest_y);
 
   /* Find middle of tiles. y-1 to not undraw the the middle pixel of a
      horizontal line when we refresh the tile below-between. */
@@ -1995,7 +1996,8 @@
     dest_x = src_x + DIR_DX[dir];
     dest_y = src_y + DIR_DY[dir];
 
-    assert(normalize_map_pos(&dest_x, &dest_y));
+    assert(is_real_tile(dest_x, dest_y));
+    normalize_map_pos(&dest_x, &dest_y);
 
     /* A previous line already marks the place */
     if (get_drawn(src_x, src_y, dir)) {
@@ -2027,7 +2029,8 @@
 
     dest_x = src_x + DIR_DX[dir];
     dest_y = src_y + DIR_DY[dir];
-    assert(normalize_map_pos(&dest_x, &dest_y));
+    assert(is_real_tile(dest_x, dest_y));
+    normalize_map_pos(&dest_x, &dest_y);
 
     assert(get_drawn(src_x, src_y, dir));
     decrement_drawn(src_x, src_y, dir);
@@ -2053,19 +2056,22 @@
 
     decrement_drawn(src_x, src_y, dir);
     refresh_tile_mapcanvas(src_x, src_y, 1);
-    assert(normalize_map_pos(&dest_x, &dest_y));
+    assert(is_real_tile(dest_x, dest_y));
+    normalize_map_pos(&dest_x, &dest_y);
     refresh_tile_mapcanvas(dest_x, dest_y, 1);
     if (NORMAL_TILE_WIDTH%2 == 0 || NORMAL_TILE_HEIGHT%2 == 0) {
       if (dir == 2) { /* Since the tle doesn't have a middle we draw an extra 
pixel
                         on the adjacent tile when drawing in this direction. */
        dest_x = src_x + 1;
        dest_y = src_y;
-       assert(normalize_map_pos(&dest_x, &dest_y));
+       assert(is_real_tile(dest_x, dest_y));
+       normalize_map_pos(&dest_x, &dest_y);
        refresh_tile_mapcanvas(dest_x, dest_y, 1);
       } else if (dir == 5) { /* the same */
        dest_x = src_x;
        dest_y = src_y + 1;
-       assert(normalize_map_pos(&dest_x, &dest_y));
+       assert(is_real_tile(dest_x, dest_y));
+       normalize_map_pos(&dest_x, &dest_y);
        refresh_tile_mapcanvas(dest_x, dest_y, 1);
       }
     }
@@ -2241,7 +2247,10 @@
     return;
   }
 
-  assert(normalize_map_pos(&x, &y));
+  /* 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);
diff -Nur -X/mnt/data/freeciv-dev/freeciv/diff_ignore 
freeciv/client/gui-mui/graphics.c codeciv/client/gui-mui/graphics.c
--- freeciv/client/gui-mui/graphics.c   Fri Jul 27 10:20:05 2001
+++ codeciv/client/gui-mui/graphics.c   Fri Jul 27 11:05:08 2001
@@ -965,7 +965,8 @@
 
   dest_x = src_x + DIR_DX[dir];
   dest_y = src_y + DIR_DY[dir];
-  assert(normalize_map_pos(&dest_x, &dest_y));
+  assert(is_real_tile(dest_x, dest_y));
+  normalize_map_pos(&dest_x, &dest_y);
 
   /* Find middle of tiles. y-1 to not undraw the the middle pixel of a
      horizontal line when we refresh the tile below-between. */
@@ -1208,7 +1209,8 @@
     return;
   }
 
-  assert(normalize_map_pos(&x, &y));
+  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);
diff -Nur -X/mnt/data/freeciv-dev/freeciv/diff_ignore 
freeciv/client/gui-mui/mapclass.c codeciv/client/gui-mui/mapclass.c
--- freeciv/client/gui-mui/mapclass.c   Fri Jul 27 10:20:05 2001
+++ codeciv/client/gui-mui/mapclass.c   Fri Jul 27 11:06:04 2001
@@ -1259,7 +1259,8 @@
          int dest_x, dest_y;
          dest_x = src_x + DIR_DX[dir];
          dest_y = src_y + DIR_DY[dir];
-         assert(normalize_map_pos(&dest_x, &dest_y));
+         assert(is_real_tile(dest_x, dest_y));
+         normalize_map_pos(&dest_x, &dest_y);
 
          /* A previous line already marks the place */
          if (get_drawn(src_x, src_y, dir)) {
@@ -1294,7 +1295,8 @@
 
          dest_x = src_x + DIR_DX[dir];
          dest_y = src_y + DIR_DY[dir];
-         assert(normalize_map_pos(&dest_x, &dest_y));
+         assert(is_real_tile(dest_x, dest_y));
+         normalize_map_pos(&dest_x, &dest_y);
 
          assert(get_drawn(src_x, src_y, dir));
          decrement_drawn(src_x, src_y, dir);
@@ -1318,19 +1320,22 @@
          } else {
            decrement_drawn(src_x, src_y, dir);
            refresh_tile_mapcanvas(src_x, src_y, 1); /* !! */
-           assert(normalize_map_pos(&dest_x, &dest_y));
+           assert(is_real_tile(dest_x, dest_y));
+           normalize_map_pos(&dest_x, &dest_y);
            refresh_tile_mapcanvas(dest_x, dest_y, 1); /* !! */
            if (NORMAL_TILE_WIDTH%2 == 0 || NORMAL_TILE_HEIGHT%2 == 0) {
              if (dir == 2) { /* Since the tle doesn't have a middle we draw an 
extra pixel
                                 on the adjacent tile when drawing in this 
direction. */
                dest_x = src_x + 1;
                dest_y = src_y;
-               assert(normalize_map_pos(&dest_x, &dest_y));
+               assert(is_real_tile(dest_x, dest_y));
+               normalize_map_pos(&dest_x, &dest_y);
                refresh_tile_mapcanvas(dest_x, dest_y, 1); /* !! */
              } else if (dir == 5) { /* the same */
                dest_x = src_x;
                dest_y = src_y + 1;
-               assert(normalize_map_pos(&dest_x, &dest_y));
+               assert(is_real_tile(dest_x, dest_y));
+               normalize_map_pos(&dest_x, &dest_y);
                refresh_tile_mapcanvas(dest_x, dest_y, 1); /* !! */
              }
            }
diff -Nur -X/mnt/data/freeciv-dev/freeciv/diff_ignore 
freeciv/client/gui-xaw/mapview.c codeciv/client/gui-xaw/mapview.c
--- freeciv/client/gui-xaw/mapview.c    Fri Jul 27 10:20:06 2001
+++ codeciv/client/gui-xaw/mapview.c    Fri Jul 27 10:53:20 2001
@@ -1312,7 +1312,8 @@
   dest_x = src_x + DIR_DX[dir];
   dest_y = src_y + DIR_DY[dir];
 
-  assert(normalize_map_pos(&dest_x, &dest_y));
+  assert(is_real_tile(dest_x, dest_y));
+  normalize_map_pos(&dest_x, &dest_y);
 
   /* A previous line already marks the place */
   if (get_drawn(src_x, src_y, dir)) {
@@ -1352,19 +1353,22 @@
 
   decrement_drawn(src_x, src_y, dir);
   refresh_tile_mapcanvas(src_x, src_y, 1);
-  assert(normalize_map_pos(&dest_x, &dest_y));
+  assert(is_real_tile(dest_x, dest_y));
+  normalize_map_pos(&dest_x, &dest_y);
   refresh_tile_mapcanvas(dest_x, dest_y, 1);
   if (NORMAL_TILE_WIDTH%2 == 0 || NORMAL_TILE_HEIGHT%2 == 0) {
     if (dir == 2) { /* Since the tle doesn't have a middle we draw an extra 
pixel
                       on the adjacent tile when drawing in this direction. */
       dest_x = src_x + 1;
       dest_y = src_y;
-      assert(normalize_map_pos(&dest_x, &dest_y));
+      assert(is_real_tile(dest_x, dest_y));
+      normalize_map_pos(&dest_x, &dest_y);
       refresh_tile_mapcanvas(dest_x, dest_y, 1);
     } else if (dir == 5) { /* the same */
       dest_x = src_x;
       dest_y = src_y + 1;
-      assert(normalize_map_pos(&dest_x, &dest_y));
+      assert(is_real_tile(dest_x, dest_y));
+      normalize_map_pos(&dest_x, &dest_y);
       refresh_tile_mapcanvas(dest_x, dest_y, 1);
     }
   }
diff -Nur -X/mnt/data/freeciv-dev/freeciv/diff_ignore freeciv/common/map.c 
codeciv/common/map.c
--- freeciv/common/map.c        Fri Jul 27 10:20:09 2001
+++ codeciv/common/map.c        Fri Jul 27 10:54:47 2001
@@ -1135,7 +1135,7 @@
 ***************************************************************/
 void map_set_continent(int x, int y, int val)
 {
-  assert(normalize_map_pos(&x, &y));
+  assert(is_real_tile(x, y));
   (map.tiles + map_adjust_x(x) + y * map.xsize)->continent=val;
 }
 
@@ -1291,5 +1291,6 @@
   *x = x0 + DIR_DX[choice];
   *y = y0 + DIR_DY[choice];
 
-  assert(normalize_map_pos(x, y));
+  assert(is_real_tile(*x, *y));
+  normalize_map_pos(x, y);
 }
diff -Nur -X/mnt/data/freeciv-dev/freeciv/diff_ignore 
freeciv/server/citytools.c codeciv/server/citytools.c
--- freeciv/server/citytools.c  Fri Jul 27 10:20:16 2001
+++ codeciv/server/citytools.c  Fri Jul 27 10:56:11 2001
@@ -1886,13 +1886,15 @@
   {
     int map_x = pcity->x + city_x - CITY_MAP_SIZE/2;
     int map_y = pcity->y + city_y - CITY_MAP_SIZE/2;
-    assert(normalize_map_pos(&map_x, &map_y));
+    assert(is_real_tile(map_x, map_y));
+    normalize_map_pos(&map_x, &map_y);
 
     map_city_radius_iterate(map_x, map_y, x1, y1) {
       struct city *pcity2 = map_get_city(x1, y1);
       if (pcity2 && pcity2 != pcity) {
        int city_x2, city_y2;
-       assert(get_citymap_xy(pcity2, map_x, map_y, &city_x2, &city_y2));
+       int res = get_citymap_xy(pcity2, map_x, map_y, &city_x2, &city_y2);
+       assert(res);
        update_city_tile_status(pcity2, city_x2, city_y2);
       }
     } map_city_radius_iterate_end;
diff -Nur -X/mnt/data/freeciv-dev/freeciv/diff_ignore freeciv/server/mapgen.c 
codeciv/server/mapgen.c
--- freeciv/server/mapgen.c     Fri Jul 27 10:20:16 2001
+++ codeciv/server/mapgen.c     Fri Jul 27 11:06:34 2001
@@ -1583,7 +1583,8 @@
       if (hmap(x, y)) {
        int map_x = x + xo - w;
        int map_y = y + yo - n;
-       assert(normalize_map_pos(&map_x, &map_y));
+       assert(is_real_tile(map_x, map_y));
+       normalize_map_pos(&map_x, &map_y);
 
        checkmass--; 
        if(checkmass<=0) {
diff -Nur -X/mnt/data/freeciv-dev/freeciv/diff_ignore 
freeciv/server/unittools.c codeciv/server/unittools.c
--- freeciv/server/unittools.c  Fri Jul 27 10:20:17 2001
+++ codeciv/server/unittools.c  Fri Jul 27 11:01:18 2001
@@ -2980,6 +2980,9 @@
   struct tile *psrctile = map_get_tile(src_x, src_y);
   struct tile *pdesttile = map_get_tile(dest_x, dest_y);
 
+  /* It is ok placing check_coords() inside an assert (ie may be optimized
+     away by compiler). Though it may adjust dest_x, dest_y it shouldn't
+     be needed. */
   assert(check_coords(&dest_x, &dest_y));
 
   conn_list_do_buffer(&pplayer->connections);