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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Aug 26, 2001 at 05:53:09PM -0400, Ross W. Wetmore wrote:
> At 11:01 PM 01/08/26 +0200, Raimar Falke wrote:
> >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:
> [...]
> >> 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.
> [...]
> >*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?!
> 
> You are probably right about that. If the tile is unreal I believe they
> get assigned as unknown, so given Gregory's point, the two are in 
> agreement and it will work. The ttype stuff is an awfully long way up 
> though :-).
> 
> >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.
> 
> No! DIR8_NORTH links this to the core direction system which might change.
> ttype_north has been used to isolate the changes to just the early section.

Than add another direction system (yes I know this is undesired) or
leave it as it is now.

> If you want to add the change to something like this, that seems fine.
>       tileno = INDEX_NSEW((ttype_north==T_UNKNOWN),
>                           (ttype_south==T_UNKNOWN),
>                           (ttype_east==T_UNKNOWN),
>                           (ttype_west==T_UNKNOWN));
> 
> And I reincluded the comments about why Thue's code needed to be duplicated 
> for the non-isometric case ... it fixes bugs that result in core files.
> 
> >I won't feel ready for another one. I will avoid a endless war.
> 
> Wise move :-). 
> 
> But fighting real code wars vs style wars might be needed once in a while.
> 
> >> >> 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.
> 
> I think unreal coordinates are probably generated by the GUI and that it
> expects it and can handle them. Any asserts I put into such places tended
> to make the GUI unusable.
> 
> But as with the corecleanups, once the underlying stuff has been fixed
> into a standard pattern, it should be possible to activate asserts like
> you suggest to catch the last remaining problems.
> 
> This is what I'm asking to happen to CVS in 07f, but it has been a couple
> months of de-crufting in parallel before I think this is reasonable. The
> GUI hasn't had that degree of workover, though Jason may be starting down
> that path, and to his credit - somebody needs to do it.
> 
> >> But FIX THE BUGS NOW :-).
> >
> >I will.
> 
> Actually that should probably be "I am", since you have gone through a
> lot with a pretty steep learning curve. But the effort needs to be 100%
> complete to make it really useful.

Step by step. My day is also only 24 hours long.

> And while I may not change my position every time, having to justify it 
> and undergo the scrutiny is really needed. I have made a number of mistakes 
> that others are catching. What you and others are doing on fairly short
> notice (I've been at it longer) is much appreciated - especially in
> calmer moments :-)

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "#!/usr/bin/perl -w
  if ( `date +%w` != 1 ) {
    die "This script only works on Mondays." ;
  }"
    -- from chkars.pl by Cornelius Krasel in de.comp.os.linux.misc