[Freeciv-Dev] Re: [PATCH] Corecleanup_07Part1 has been put in incoming

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Aug 26, 2001 at 10:50:00AM -0400, Ross W. Wetmore wrote:
> At 03:20 PM 01/08/26 +0200, Raimar Falke wrote:
> >On Sat, Aug 25, 2001 at 07:02:22PM -0400, Ross W. Wetmore wrote:
> >> Subject:  [Patch4cvs] Corecleanup_07e patch
> >> Date:     Aug 26, 2001
> >> Author:   Ross Wetmore
> >> Status:   Tested
> >> State:    ReSubmission of corecleanup_06a
> >> 
> >> Description
> >>   This patch contains all miscellaneous and client-GUI changes.
> >> 
> >> Dependencies
> >>   corecleanup_07a: map.h
> >> 
> >> Files Changed:
> >> corecleanup-07e: freeciv/client/gui-gtk/mapview.c
> >> corecleanup-07e: freeciv/client/gui-xaw/mapview.c
> >> corecleanup-07e: freeciv/client/tilespec.c
> >> corecleanup-07e: freeciv/server/sanitycheck.c
> >
> >The tilespec changes doesn't make the code any cleaner/less ugly. I'm
> >not sure about the changes to mapview.c. We should decide what
> >position are legal and which aren't.
> 
> Which ones, or what exactly? Vague innuendos are difficult to respond to.

+  /* TODO: consolidate with GUI Dir system, Dir enums for magic no's */
+  for (dir=0; dir<8; dir++) {
+    int x1 = abs_x0 + DIR_DX2[dir];
+    int y1 = abs_y0 + DIR_DY2[dir];
+    if (normalize_map_pos(&x1, &y1)) {
+      ttype_near[dir] = map_get_terrain(x1, y1);
+      tspecial_near[dir] = map_get_special(x1, y1);
+    } else {
+      ttype_near[dir] = T_UNKNOWN;
+      tspecial_near[dir] = S_NO_SPECIAL;
+    }
+  }
   ttype=map_get_terrain(abs_x0, abs_y0);
-  ttype_east=map_get_terrain(abs_x0+1, abs_y0);
-  ttype_west=map_get_terrain(abs_x0-1, abs_y0);
+  tspecial=map_get_special(abs_x0, abs_y0);
+
+  ttype_north      = ttype_near[0];
+  ttype_north_east = ttype_near[1];
+  ttype_east       = ttype_near[2];
+  ttype_south_east = ttype_near[3];
+  ttype_south      = ttype_near[4];
+  ttype_south_west = ttype_near[5];
+  ttype_west       = ttype_near[6];
+  ttype_north_west = ttype_near[7];
+  tspecial_north      = tspecial_near[0];
+  tspecial_north_east = tspecial_near[1];
+  tspecial_east       = tspecial_near[2];
+  tspecial_south_east = tspecial_near[3];
+  tspecial_south      = tspecial_near[4];
+  tspecial_south_west = tspecial_near[5];
+  tspecial_west       = tspecial_near[6];
+  tspecial_north_west = tspecial_near[7];

First you introduce an array (nice) and later you copy the stuff to a
variable. Either leave this out or go the whole distance (i.e. have
ttype_near[DIR8_NORTH] instead of ttype_north).

Also these changes:
-    tileno = INDEX_NSEW((tile_is_known(abs_x0, abs_y0-1)==TILE_UNKNOWN),
-                        (tile_is_known(abs_x0, abs_y0+1)==TILE_UNKNOWN),
-                        (tile_is_known(abs_x0+1, abs_y0)==TILE_UNKNOWN),
-                        (tile_is_known(abs_x0-1, abs_y0)==TILE_UNKNOWN));
+    tileno = INDEX_NSEW(
+      (!is_real_tile(abs_x0, abs_y0-1)
+        || (tile_is_known(abs_x0, abs_y0-1)==TILE_UNKNOWN)),
+      (!is_real_tile(abs_x0, abs_y0+1)
+        || (tile_is_known(abs_x0, abs_y0+1)==TILE_UNKNOWN)),
+      (!is_real_tile(abs_x0+1, abs_y0)
+        || (tile_is_known(abs_x0+1, abs_y0)==TILE_UNKNOWN)),
+      (!is_real_tile(abs_x0-1, abs_y0)
+        || (tile_is_known(abs_x0-1, abs_y0)==TILE_UNKNOWN)));

don't make the code prettier. Yes it may needed but you can also do
this in a nice form.

> I assure you that without most of those changes it is easy to generate
> core files. And things like added comments were requested.

As I said: before we add comments like:
-  dest_x = map_adjust_x(x0+dx);
-  dest_y = map_adjust_y(y0+dy);
+  /* TODO: if this returns false, tile is bad. Shouldn't we just return? */
+  dest_x = (x0+dx);
+  dest_y = (y0+dy);
+  normalize_map_pos(&dest_x,&dest_y);

Can't we just declare this methods needs real tiles and this method
doesn't?! Or at least make it an assert so that this comes up later. I
just fear that not many people read comments. There are 126 fixmes and
19 todos in the source.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "From what I am reading Win98 and NT5.0 will be getting rid of all that
  crap anyway. Seems that Microsoft has invented something called TCP/IP and
  another really revolutionary concept called DNS that eliminates the
  netbios crap too. All that arping from browsers is going to go away.
  I also hear rumors that they are on the verge of breakthrough discoveries
  called NFS, and LPD too. Given enough time and money, they might
  eventually invent Unix."
    -- George Bonser in linux-kernel