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

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

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.

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.

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

Cheers,
RossW