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

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

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?

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 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.

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 :-).

>       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

Cheers,
RossW
=====