[Freeciv-Dev] Re: is_real_tile -> is_real_map_pos

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

This should not be just a rename, but fix the definition and use as well.

In many cases, the same code is being called 2 or 3 times. In others
the call should be changed to be normalize_map_pos() since this provides
more safety at reduced cost.

The rename is such a low priority on its own, that one wonders why take
the effort to do things twice.

But there is nothing inherently wrong with the consistency change. 

At 06:30 AM 02/01/11 -0500, Jason Short wrote:
>It may be time to consider renaming is_real_tile as is_real_map_pos.  I 
>think we all agree is_real_map_pos is the correct name; the only 
>question is whether the name change is worth the intrusiveness of (gasp) 
>such a massive patch.
>
>This patch also provides a function header comment for is_real_map_pos. 
>  Also attached is a patch that *just* gives the header comment, without 
>the rename.
>
>jason
>Index: common/map.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
>retrieving revision 1.103
>diff -u -r1.103 map.c
>--- common/map.c       2001/12/13 19:13:16     1.103
>+++ common/map.c       2002/01/11 11:27:17
>@@ -1241,6 +1241,17 @@
>   return (x1 == x2 && y1 == y2);
> }
> 
>+
>+/**************************************************************************
>+  Returns TRUE iff the given map position (x, y) is real.
>+
>+  Realness is a "hard" condition; any map position that is real refers to
>+  an actual tile.  In the current cylindrical topology, a map position is
>+  real if 0 <= y <= map.ysize.  Any map positions outside this range are
>+  unreal and generally useless for everything.  Positions within this
>+  range are always usable, but may need to be normalized (see
>+  normalize_map_pos) before they are directly usable.
>+**************************************************************************/
> int is_real_tile(int x, int y)
> {
>   return normalize_map_pos(&x, &y);

This is a test condition. It is a initial condition of normalize_map_pos(),
in that normalize_map_pos() should not waste time or change values that 
cause side effects if the position is unreal.

Calling normalize_map_pos() is both backwards, and inefficient in the 
case where it returns real and takes a full normalization cycle to 
generate normal coordinates that are thrown away.

The test is a simple condition, and a full function call is not required.

This should be restored to the earlier CVS flavour.

>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      2002/01/11 11:19:27
>@@ -618,8 +618,8 @@
> {
>   int x1, y1, is_real;
> 
>-  /* Replace with check for is_normal_tile later */  
>-  assert(is_real_tile(x, y));
>+  /* Replace with check for is_normal_tile later */
>+  assert(is_real_map_pos(x, y));
>   normalize_map_pos(&x, &y);
> 
>   is_real = MAPSTEP(x1, y1, x, y, dir);
>@@ -798,7 +798,7 @@
>   assert(is_active);
> 
>   /* Replace with check for is_normal_tile later */
>-  assert(is_real_tile(dest_x, dest_y));
>+  assert(is_real_map_pos(dest_x, dest_y));
>   normalize_map_pos(&dest_x, &dest_y);
> 
>   if (!goto_map.vector[dest_x][dest_y]) {

The construct here is 

  is_real = normalize_map_pos(&x,&y);
  assert(is_real);

This has the advantage of executing normalize_map_pos() once in the
current flavour, and not twice. 

Even in the case where the assert is triggered, the time saved by
doing an initial realness check before normalizing is not significant.

>Index: client/mapview_common.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/mapview_common.c,v
>retrieving revision 1.4
>diff -u -r1.4 mapview_common.c
>--- client/mapview_common.c    2001/12/21 16:53:10     1.4
>+++ client/mapview_common.c    2002/01/11 11:19:28
>@@ -28,7 +28,7 @@
> **************************************************************************/
> void refresh_tile_mapcanvas(int x, int y, int write_to_screen)
> {
>-  assert(is_real_tile(x, y));
>+  assert(is_real_map_pos(x, y));
>   if (!normalize_map_pos(&x, &y)) {
>     return;
>   }
>@@ -51,9 +51,9 @@
>       player_in_city_radius(game.player_ptr, x1, y1);
>   int pos2_is_in_city_radius = 0;
> 
>-  assert(is_real_tile(x1, y1));
>+  assert(is_real_map_pos(x1, y1));
> 
>-  if (is_real_tile(x2, y2)) {
>+  if (is_real_map_pos(x2, y2)) {
>     normalize_map_pos(&x2, &y2);
>     assert(is_tiles_adjacent(x1, y1, x2, y2));

is_real_tile should be removed and replaced by normalize_map_pos()
in the conditional.

If the assert is required, then the "is_real =" can be used both in
the assert and in the conditional.

Note normalize_map_pos() is now called three times here.
 
>Index: client/gui-gtk/dialogs.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/dialogs.c,v
>retrieving revision 1.80
>diff -u -r1.80 dialogs.c
>--- client/gui-gtk/dialogs.c   2001/12/21 16:26:38     1.80
>+++ client/gui-gtk/dialogs.c   2002/01/11 11:19:30
>@@ -312,7 +312,7 @@
>   GtkWidget *notify_dialog_shell, *notify_command, *notify_goto_command;
>   GtkWidget *notify_label;
>   
>-  if (!is_real_tile(x, y)) {
>+  if (!is_real_map_pos(x, y)) {
>     popup_notify_dialog(_("Message:"), headline, lines);
>     return;
>   }
>Index: client/gui-gtk/mapview.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/mapview.c,v
>retrieving revision 1.111
>diff -u -r1.111 mapview.c
>--- client/gui-gtk/mapview.c   2001/12/21 19:14:18     1.111
>+++ client/gui-gtk/mapview.c   2002/01/11 11:19:33
>@@ -208,7 +208,7 @@
> 
>   if (!citymode) {
>     /* put any goto lines on the tile. */
>-    if (is_real_tile(x, y)) {
>+    if (is_real_map_pos(x, y)) {
>       int dir;
>       for (dir = 0; dir < 8; dir++) {
>       if (get_drawn(x, y, dir)) {
>@@ -2094,7 +2094,7 @@
>   }
> 
>   /* Replace with check for is_normal_tile later */
>-  assert(is_real_tile(x, y));
>+  assert(is_real_map_pos(x, y));
>   normalize_map_pos(&x, &y);
> 
>   fog = tile_get_known(x, y) == TILE_KNOWN_FOGGED && draw_fog_of_war;

Use the is_real flavour here.

>Index: client/gui-mui/graphics.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/gui-mui/graphics.c,v
>retrieving revision 1.19
>diff -u -r1.19 graphics.c
>--- client/gui-mui/graphics.c  2002/01/09 18:44:47     1.19
>+++ client/gui-mui/graphics.c  2002/01/11 11:19:34
>@@ -980,7 +980,7 @@
> 
>   if (!citymode) {
>     /* put any goto lines on the tile. */
>-    if (is_real_tile(x, y)) {
>+    if (is_real_map_pos(x, y)) {
>       int dir;
>       for (dir = 0; dir < 8; dir++) {
>       if (get_drawn(x, y, dir)) {
>@@ -1249,7 +1249,7 @@
>     return;
>   }
> 
>-  assert(is_real_tile(x, y));
>+  assert(is_real_map_pos(x, y));
>   normalize_map_pos(&x, &y);
>   fog = tile_get_known(x, y) == TILE_KNOWN_FOGGED && draw_fog_of_war;
>   pcity = map_get_city(x, y);

Use the is_real flavour here.

>Index: client/gui-win32/mapview.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/gui-win32/mapview.c,v
>retrieving revision 1.19
>diff -u -r1.19 mapview.c
>--- client/gui-win32/mapview.c 2001/12/18 13:52:36     1.19
>+++ client/gui-win32/mapview.c 2002/01/11 11:19:37
>@@ -358,7 +358,7 @@
> 
>   if (!citymode) {
>     /* put any goto lines on the tile. */
>-    if (is_real_tile(x, y)) {
>+    if (is_real_map_pos(x, y)) {
>       int dir;
>       for (dir = 0; dir < 8; dir++) {
>         if (get_drawn(x, y, dir)) {
>Index: client/gui-xaw/dialogs.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/gui-xaw/dialogs.c,v
>retrieving revision 1.59
>diff -u -r1.59 dialogs.c
>--- client/gui-xaw/dialogs.c   2001/12/21 16:26:40     1.59
>+++ client/gui-xaw/dialogs.c   2002/01/11 11:19:39
>@@ -311,7 +311,7 @@
>   Widget notify_headline, notify_label;
>   Dimension width, width2, width_1, width_2;
>   
>-  if (!is_real_tile(x, y)) {
>+  if (!is_real_map_pos(x, y)) {
>     popup_notify_dialog("Message:", headline, lines);
>     return;
>   }
>Index: client/gui-xaw/mapview.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/gui-xaw/mapview.c,v
>retrieving revision 1.92
>diff -u -r1.92 mapview.c
>--- client/gui-xaw/mapview.c   2002/01/11 04:24:13     1.92
>+++ client/gui-xaw/mapview.c   2002/01/11 11:19:41
>@@ -1057,7 +1057,7 @@
> 
>   if (!citymode) {
>     /* put any goto lines on the tile. */
>-    if (is_real_tile(x, y)) {
>+    if (is_real_map_pos(x, y)) {
>       int dir;
>       for (dir = 0; dir < 8; dir++) {
>       if (get_drawn(x, y, dir)) {
>Index: common/city.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/common/city.c,v
>retrieving revision 1.131
>diff -u -r1.131 city.c
>--- common/city.c      2002/01/10 23:14:03     1.131
>+++ common/city.c      2002/01/11 11:19:43
>@@ -109,7 +109,7 @@
>                        int city_center_x, int city_center_y,
>                        int map_x, int map_y)
> {
>-  assert(is_real_tile(map_x, map_y));
>+  assert(is_real_map_pos(map_x, map_y));
>   city_map_checked_iterate(city_center_x, city_center_y, cx, cy, mx, my) {
>     if (mx == map_x && my == map_y) {
>       *city_map_x = cx;

The assert is not needed here. Unreal positions will not be in the citymap
so there is no code to protect here.

>@@ -2251,7 +2251,7 @@
>                               *result_city_tile_type,
>                               struct city **result_pcity)
> {
>-  assert(is_real_tile(map_x, map_y));
>+  assert(is_real_map_pos(map_x, map_y));
> 
>   *result_pcity = map_get_tile(map_x, map_y)->worked;
>   if (*result_pcity) {


This seems to be included twice.

>Index: common/map.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
>retrieving revision 1.103
>diff -u -r1.103 map.c
>--- common/map.c       2001/12/13 19:13:16     1.103
>+++ common/map.c       2002/01/11 11:19:44
>@@ -1018,7 +1018,7 @@
> static int tile_move_cost_ai(struct tile *tile0, struct tile *tile1,
>                            int x, int y, int x1, int y1, int maxcost)
> {
>-  assert(is_real_tile(x, y));
>+  assert(is_real_map_pos(x, y));
>   assert(!is_server
>        || (tile0->terrain != T_UNKNOWN && tile1->terrain != T_UNKNOWN));
> 
>@@ -1241,7 +1241,17 @@
>   return (x1 == x2 && y1 == y2);
> }
> 
>-int is_real_tile(int x, int y)
>+/**************************************************************************
>+  Returns TRUE iff the given map position (x, y) is real.
>+
>+  Realness is a "hard" condition; any map position that is real refers to
>+  an actual tile.  In the current cylindrical topology, a map position is
>+  real if 0 <= y <= map.ysize.  Any map positions outside this range are
>+  unreal and generally useless for everything.  Positions within this
>+  range are always usable, but may need to be normalized (see
>+  normalize_map_pos) before they are directly usable.
>+**************************************************************************/
>+int is_real_map_pos(int x, int y)
> {
>   return normalize_map_pos(&x, &y);
> }
>@@ -1343,7 +1353,7 @@
>     DIR8_SOUTHWEST, DIR8_SOUTH, DIR8_SOUTHEAST
>   };
> 
>-  assert(is_real_tile(x0, y0));
>+  assert(is_real_map_pos(x0, y0));
> 
>   /* This clever loop by Trent Piepho will take no more than
>    * 8 tries to find a valid direction. */
>Index: common/map.h
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
>retrieving revision 1.109
>diff -u -r1.109 map.h
>--- common/map.h       2002/01/06 16:43:26     1.109
>+++ common/map.h       2002/01/11 11:19:45
>@@ -282,7 +282,7 @@
> void map_set_special(int x, int y, enum tile_special_type spe);
> void map_clear_special(int x, int y, enum tile_special_type spe);
> void tile_init(struct tile *ptile);
>-int is_real_tile(int x, int y);
>+int is_real_map_pos(int x, int y);
> int is_normal_map_pos(int x, int y);
> 
> /* 
>Index: server/barbarian.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/server/barbarian.c,v
>retrieving revision 1.40
>diff -u -r1.40 barbarian.c
>--- server/barbarian.c 2001/12/11 16:16:47     1.40
>+++ server/barbarian.c 2002/01/11 11:19:47
>@@ -152,7 +152,7 @@
> **************************************************************************/
> static int is_free_land(int x, int y, struct player *who)
> {
>-  if (!is_real_tile(x, y) || map_get_terrain(x, y) == T_OCEAN
>+  if (!is_real_map_pos(x, y) || map_get_terrain(x, y) == T_OCEAN
>       || is_non_allied_unit_tile(map_get_tile(x, y), who))
>     return 0;
>   else

This should be normalize_map_pos(). This will protect the code
following if unnormal locations are passed at no additional expense.

Currently they will fail possibly unpredictably on unnormal access.

>@@ -164,7 +164,7 @@
> **************************************************************************/
> static int is_free_sea(int x, int y, struct player *who)
> {
>-  if (!is_real_tile(x, y) || map_get_terrain(x, y) != T_OCEAN
>+  if (!is_real_map_pos(x, y) || map_get_terrain(x, y) != T_OCEAN
>       || is_non_allied_unit_tile(map_get_tile(x, y), who))
>     return 0;
>   else

ditto

>Index: server/gamehand.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/server/gamehand.c,v
>retrieving revision 1.100
>diff -u -r1.100 gamehand.c
>--- server/gamehand.c  2002/01/09 21:48:10     1.100
>+++ server/gamehand.c  2002/01/11 11:19:47
>@@ -113,7 +113,7 @@
>         dx = x + myrand(2 * game.dispersion + 1) - game.dispersion;
>         dy = y + myrand(2 * game.dispersion + 1) - game.dispersion;
>         normalize_map_pos(&dx, &dy);
>-      } while (!(is_real_tile(dx, dy)
>+      } while (!(is_real_map_pos(dx, dy)
>                    && map_get_continent(x, y) == map_get_continent(dx, dy)
>                    && map_get_terrain(dx, dy) != T_OCEAN
>                    && !is_non_allied_unit_tile(map_get_tile(dx, dy),

The normalize_map_pos() should be moved into the while and the 
is_real_tile() removed.

>Index: server/maphand.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/server/maphand.c,v
>retrieving revision 1.92
>diff -u -r1.92 maphand.c
>--- server/maphand.c   2002/01/09 21:48:11     1.92
>+++ server/maphand.c   2002/01/11 11:19:48
>@@ -943,7 +943,7 @@
> struct player_tile *map_get_player_tile(int x, int y,
>                                       struct player *pplayer)
> {
>-  if (!is_real_tile(x, y)) {
>+  if (!is_real_map_pos(x, y)) {
>     freelog(LOG_ERROR, "Trying to get nonexistant tile at %i,%i", x, y);
>   }
>   nearest_real_pos(&x, &y);
>Index: server/plrhand.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/server/plrhand.c,v
>retrieving revision 1.204
>diff -u -r1.204 plrhand.c
>--- server/plrhand.c   2002/01/09 21:48:11     1.204
>+++ server/plrhand.c   2002/01/11 11:19:50
>@@ -905,7 +905,7 @@
>   genmsg.event = event;
> 
>   conn_list_iterate(*dest, pconn) {
>-    if (is_real_tile(x, y) && server_state >= RUN_GAME_STATE
>+    if (is_real_map_pos(x, y) && server_state >= RUN_GAME_STATE
>       && ((pconn->player==NULL && pconn->observer)
>           || (pconn->player!=NULL && map_get_known(x, y, pconn->player)))) {
>       genmsg.x = x;

This is already being fixed to reverse the server_state and is_real check
trap the (-1,-1) signal position and call normalize_map_pos() in place
of is_real_tile() to protect the map_get_known() call.

>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       2002/01/11 11:19:50
>@@ -210,7 +210,7 @@
>       struct city *pcity;
> 
>       assert(unit_owner(punit) == pplayer);
>-      assert(is_real_tile(x, y));
>+      assert(is_real_map_pos(x, y));
> 
>       if (punit->homecity) {
>       pcity = player_find_city_by_id(pplayer, punit->homecity);
>