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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Aug 26, 2001 at 03:57:33PM -0400, Ross W. Wetmore wrote:
> At 06:45 PM 01/08/26 +0200, Raimar Falke wrote:
> >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
> [...]
> >> >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).
> 
> This duplicates Thue's fixes for isometric in the standard section. It
> fixes the hardcoded unchecked accesses to map routines with an adjacent
> loop, which is what the current system does.
> 
> This also means that when the GUI cleanups fix Thue's first change as 
> you suggest, they should also see this one and not miss the connection.
> 
> Rejecting this is a case of leaving broken code, because the good does
> not seem to be quite perfect.
> >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.
> 
> This is as nice a form as any :-).
> 
> Gregory pointed out that the previous pass did not treat unreal tiles
> correctly, though it fixed the core from unchecked access. The above
> code actually does exactly what you would describe as needed if you
> translated the code to English.
>   "unreal tiles or unknown tiles in each cardinal direction are bit
>   adjusted by the INDEX_NSEW macro to form the sprite array index".
> 
> You should first concentrate on fixing code problems, and then on 
> cosmetic improvements for which there seem to be a number of unresolvable
> subjective concepts of "beauty", no? Is this not your stated position?

*looking at the code* ok it really looks like there is no nice
solution. The conditions for INDEX_NSEW vary too much. The only thing
I found is:

    if (ttype == T_HILLS) {
      tileno = INDEX_NSEW((ttype_north==T_HILLS || ttype_north==T_HILLS),
                          (ttype_south==T_HILLS || ttype_south==T_HILLS),
                          (ttype_east==T_HILLS || ttype_east==T_HILLS),
                          (ttype_west==T_HILLS || ttype_west==T_HILLS));
      *sprs++=sprites.tx.spec_hill[tileno];
    }

One of ttype_east==T_HILLS should be enough?!

Summary: The extra checks are ok. The changes you made to duplicate
Thue's changes are unnecessary for the cleanup. IMHO this can really
be expresses more nicely with ttype_near[DIR8_NORTH] for example.

> As a guiding rule, maybe ... If it conforms to K&R2, or is consistent with
> the current (surrounding) code, it must be accepted. If there is general 
> agreement for changing the appearance of the current code and a set of 
> published style guidelines that can be used to measure acceptable changes, 
> then that is ok as well.
> 
> Othewise, you are going to have endless "style" wars.

I won't feel ready for another one. I will avoid a endless war.

> >> I assure you that without most of those changes it is easy to generate
> >> core files. And things like added comments were requested.
> 
> And you have left significant bugs in the code by dropping some of these
> changes. 
> 
> >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);
> 
> This will also cause an assert if map_adjust_y is set to trap off map 
> accesses for non-wrap coordinates.
> 
> The normalize_map_pos fix is the correct (and valid for later topologies)
> fix to apply here. The additional comment serves to remind those doing
> GUIcleanup that there might be something to look at here.
> 
> But since it works fine and doesn't dump core with the fix, one can 
> possibly assume that the original GUI person did think this case through.

So can you put an assert there? This serves as an implicit comment.

> In any event cleaning this up is part of the overall GUI cleanups that
> are needed and fixing the core dump is all that is required for the 
> corecleanup.
> 
> >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.
> 
> I repeat ... I repeat ... I repeat ... The GUI needs to be looked at in
> a comprehensive way. One should do the absolute minimal consistent set of
> changes during corecleanups to not destabilize it. 
> 
> I have no problem with fixing this, just not now, and not without some
> real thought.
> 
> But FIX THE BUGS NOW :-).

I will.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "There are three ways to get something done. Do it yourself, hire someone
  to do it for you or forbid your kids to do it."