[Freeciv-Dev] Re: PATCH: remove map_adjuxt_[xy] invocations

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

I would not clutter map_inx(), which was supposed to be changed to 
map_index(), with a lot of extra checks and stuff.

Make a map_index_checked() for this purpose, and put it where it 
is needed.

Some minor nits below.

Cheers,
RossW
=====

At 01:03 AM 01/10/03 -0400, Jason Dorje Short wrote:
>[I tried to post this to freeciv-bugs much earlier today, but with still
>no success I think I've got some problem at my end.  Well, here it is.]
>
>The attached patch removes all remaining invocations of map_adjust_[xy]
>in server code.
>
>It has a bit of clutter, though: for one thing, it adds a function
>is_normal_map_pos, which may still be controversial.  I also think the
>assertions used in hmap, map_get_tile, and elsewhere would be better to
>put into map_inx (a question for later).
>
>The most questionable bit of code is the usage of hmap, but using
>adjc_iterate should be safe.
>
>As before, it should be safe to patch each file independently.
>
>jasonIndex: client/control.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/control.c,v
>retrieving revision 1.60
>diff -u -u -5 -r1.60 control.c
>--- client/control.c   2001/09/15 15:31:19     1.60
>+++ client/control.c   2001/10/02 15:44:40
>@@ -592,16 +592,16 @@
>   a dialog. (the server code has to be there anyway as goto's are entirely
>   in the server)
> **************************************************************************/
> void request_move_unit_direction(struct unit *punit, int dx, int dy)
> {
>-  int dest_x, dest_y;
>+  int dest_x = punit->x + dx;
>+  int dest_y = punit->y + dy;
>   struct unit req_unit;
> 
>-  dest_x = map_adjust_x(punit->x+dx);
>-  dest_y = punit->y+dy; /* Not adjusted since if it needed to be adjusted it
>-                         would mean that we tried to move off the map... */
>+  assert(is_real_tile(dest_x, dest_y));
>+  normalize_map_pos(&dest_x, &dest_y);
> 
>   /* Catches attempts to move off map */
>   if (!is_real_tile(dest_x, dest_y))
>     return;

These 3 lines are completely redundant. Either put the normalize_map_pos()
after them and remove the assert, or remove them.

> 
>@@ -1066,11 +1066,13 @@
>     if(y<0 || y>=map.ysize)
>       continue;

This is another partial is_real_tile(). But in fact this whole block
is a square_iterate() and these changes should be replaced with the
proper checked iterate macro. Your assert is in fact wrong here.

>     for(x=punit->x-2; x<punit->x+3; ++x) { 
>       unit_list_iterate(map_get_tile(x, y)->units, pu)
>       if(unit_flag(pu, F_PARTIAL_INVIS)) {
>-        refresh_tile_mapcanvas(map_adjust_x(pu->x), y, 1);
>+        /* Why not just use (x, y) instead of (pu->x, pu->y)?  --JDS */
>+        assert(is_normal_map_pos(pu->x, pu->y));
>+        refresh_tile_mapcanvas(pu->x, pu->y, 1);
>       }
>       unit_list_iterate_end
>     }
>   }
>   
>Index: server/mapgen.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/server/mapgen.c,v
>retrieving revision 1.72
>diff -u -u -5 -r1.72 mapgen.c
>--- server/mapgen.c    2001/09/30 21:55:34     1.72
>+++ server/mapgen.c    2001/10/02 15:45:03
>@@ -1336,33 +1338,23 @@
>   smooth_map should be viewed  as a  corrosion function on the map, it
levels
>   out the differences in the heightmap.
> **************************************************************************/
> static void smooth_map(void)
> {
>-  int mx,my,px,py;
>   int a;
>-  
>+
>+  /* This overwrites itself as it runs, creating an unpredictable
>+     feedback.  Is this desired?  --JDS */
>   whole_map_iterate(x, y) {
>-    my = map_adjust_y(y - 1);
>-    py = map_adjust_y(y + 1);
>-    mx = map_adjust_x(x - 1);
>-    px = map_adjust_x(x + 1);
>     a = hmap(x, y) * 2;
>-
>-    a += hmap(px, my);
>-    a += hmap(mx, my);
>-    a += hmap(mx, py);
>-    a += hmap(px, py);
> 
>-    a += hmap(x, my);
>-    a += hmap(mx, y);
>+    adjc_iterate(x, y, x2, y2) {
>+      a += hmap(x2, y2);
>+    } adjc_iterate_end;
> 
>-    a += hmap(x, py);
>-    a += hmap(px, y);
>+    a += myrand(60) - 30;
> 
>-    a += myrand(60);
>-    a -= 30;
>     if (a < 0)
>       a = 0;
>     hmap(x, y) = a / 10;

Your use of adjacent_iterate means the /10 is incorrect. You should
probably accumulate the count of tiles actually collected.

>   } whole_map_iterate_end;
> }
>Index: server/unittools.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
>retrieving revision 1.138
>diff -u -u -5 -r1.138 unittools.c
>--- server/unittools.c 2001/09/15 15:31:28     1.138
>+++ server/unittools.c 2001/10/02 15:45:41
>@@ -1625,12 +1625,16 @@
> 
>   punit->type=type;
>   punit->id=get_next_id_number();
>   idex_register_unit(punit);
>   punit->owner=pplayer->player_no;
>-  punit->x = map_adjust_x(x); /* was = x, caused segfaults -- Syela */
>-  punit->y=y;
>+
>+  assert(is_real_tile(x, y));
>+  normalize_map_pos(&x, &y); /* Should be handled by caller.  --JDS */
>+  punit->x = x;
>+  punit->y = y;
>+
>   if (y < 0 || y >= map.ysize) {
>     freelog(LOG_ERROR, "Whoa!  Creating %s at illegal loc (%d, %d)",
>           get_unit_type(type)->name, x, y);
>   }

This is another partial is_real_tile() check that in now redundant after
the assert. Either move this up and ditch the assert or ditch this.

>   punit->goto_dest_x=0;
>
>