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

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

At 10:10 AM 01/08/29 +0200, Raimar Falke wrote:
>On Tue, Aug 28, 2001 at 10:55:47PM -0400, Ross W. Wetmore wrote:
>> At 06:29 PM 01/08/28 +0200, Raimar Falke wrote:
>> >On Sun, Aug 26, 2001 at 03:17:37PM -0400, Ross W. Wetmore wrote:
>> > - there should be no *_x and *_y version just one which takes both
>> > coordinates
>> 
>> There appears to be some context missing here. Are you referring to
>> map_adjust_x() and map_adjust_y()? These are untouched versions of the 
>> CVS functions at this point, I believe.
>
>Yes but I hoped to get rid of them and you don't touch them.

So you *do* want my patches to be bigger to deal with this :-).

It would be better to comment on and apply what is in submitted patches
and not what could be in them or what one might like to be in them.

It is entirely reasonable to make suggestions for other things even when
they are not really part of the patch and significant amounts of work. But
do it by calling for new patches, or deal with this separately as it 
warrants.

>> > - +#define FC__CITY_C     1        /* pickup city_map_indices */
>> >   +#include "city.h"
>> >   This is ugly. Why?
> Why are you using "#define FC__CITY_C 1" if this isn't
>needed in the current version?

It is needed, look about a dozen lines down the diff, then in city.h.

>You have to acknowledge that casting to unsigned and comparing it is a
>trick to gain performance and this trick isn't immediate obvious to a
>normal programmer. It has to be documented somewhere or we went up
>like in the ai case.

No, it can be viewed as *understanding* that real map coordinates are 
always positive and defined by an upper bound and doing the appropriate 
logical test for this condition. It is unfortunate that map coordinates
were stored in ints instead of unsigned, but the cast corrects this.

This is not some obscure hack like adding a truncate to map_adjust_y
to stop unreal coordinates from causing core faults by instead pointing 
them at bordertiles.

I accept the comment about documenting it somewhere though.

But I also don't believe one has to program inefficiently to prove that
one's coding style is better, whether it be adding unnecessary loops, 
tests or doing things like putting 4 line for blocks into functions and 
hiding them away for inexplicable (and undocumented) reasons :-). Things
like this, I've had to merge around in the last few CVS updates.

>> > - make a is_normal(ized)_position
>> > - insert an "assert(is_normal(ized)_position);" into every access
>> > method of map.h
>> > - debug
>> 
>> I will first point out that assert is a void function, and thus cannot
>> be inserted into macro expressions. There are no access macro methods
>> returning values in map.h anyway.
>
>map_get_tile is defined/declared in map.h

map_get_tile is an extern function in map.h, it is not possible to put
an assert into an extern declaration either.

>> 07f does something like this to the access "functions" in map.c. It does
>> not to check whether a position is "normalized" or not as the current 
>> functions (or majority of them) assumed the need to normalize, and there
>> is a lot of code like the tilespec example awhile back that require this. 
>> This behaviour is not being changed in CVS by any of the current patches.
>> 
>> To restate this, there has been no effort to insure that coordinates are 
>> normalized before passing them into functions, and thus the suggested test 
>> would break CVS badly at the current time. I strongly recommend against 
>> such a rash action.
>
>I didn't vote for inserting them in the cvs and then debug it. Insert,
>debug, commit.

You should add this to your request list as a new cleanup effort and patch.

>> 07f does an assert for "unreal" at this point, which is the state at 
>> which the current cleanups are safe except for residual nits. This is 
>> the requirements set for the past couple months work and reason for
>> submission of this series of patches.
>
>> It is all that is also required to handle alternate topologies, whereas
>> the is_normal(ized) check is purely for the performance optimization of 
>> doing these checks once at source, rather than at every level.
>
>Can you explain this in more detail?

Alternate topologies require proper is_real_tile() and normalize_map_pos()
handling to deal with optional coordinate wrapping. When off map coordinates 
can't be wrapped they result in core dumps caused by access to unreal map
positions. So iteration and stepping code needs to deal with these. This
is a large part of the corecleanup bugfixing.

The random use of map_adjust_y and assumptions about when it could be
omitted, 
truncated or used as half of a normalize_map_pos operation without considering
that this is a realness test in the standard map and quite different from the
wrapping test in map_adjust_x, are a second major cleanup.

Understanding realness is about all that is stopping one from running 
alternate topologies. Flat-earth is the best debugging ground for catching
realness bugs, and Donut-world is worthless for this, i.e. it is always
safe. And the changes to add has_mapwrap in 07l if you grep may be
surprisingly few considering what is fixed in 07a-k, where the map is
still standard MAP_WRAP_X.

The problems with normalizing are perhaps the other way, in that this is
done everywhere rather than at the top or bottom of the call chain. The
issue with normalizing is mostly one of performance, not correctness.

So you want to add the assert conditions for the acces functions in map.c
that are in 07f. They test for realness, which is pretty much fixed if you
did a complete job of updating CVS with the earlier patches.

You should not care about is_normalized asserts until you are ready for a
big performance and profiling effort, and there will be a lot of code
fixes to change the current codebase to understand that coordinates are
passed around in only normalized form. The current rule only guarantees
that passed coordinates be real.

>> The first step is to make sure things are real. 
>> 
>> With the patches submissions prior to 07f this is expected to be complete
>> and 07f will test this in DEBUG mode. If anything has been omitted, 07f
>> should find it pretty quickly.
>
>That is another nice idea. Putting the checks in the current CVS and
>you have to enable them.

So stop wasting time and put them in :-).

>> To a large extent these are removed by the time you apply all the patches
>> as instructed. But in the GUI code in particular, this needs to be done in
>> a more comprehensive manner as part of some careful overhauls of its
>> assumptions about map topologies.
>
>Mhh still haven't looked at corecleanup_07e a second time

I'd take a second look at anything you dropped from a-e to make sure it
did not fix an unreal tile condition. But you can apply 07f and test using
autogame to see how long you last. I've been running Flat-Earth autogames
in a while loop since the last patch was submitted on Sunday. So I'm quite
confident of my changes.

It wouldn't hurt to patch a cvs-Aug-25 with everything, and look at
side-by-side file comparisons instead of diffs to get a good feel for
the concepts, the basic things you want done differently, the few 
things you should not want done. You can also run the map_type autogames
here to test some of your ideas and changes.

Note, you can roll the patches in and out quite readily if you follow
the proper order, and this is primarily determined by file collisions.
So it is easy to go back do a `make clean; make` to see differnt stages.

Then if you do go back to the diffs and merge them into CVS you won't 
inadvertently get part of a fix because things were not fully understood.

>> So if you have completed your review of either the Part2 or Parts 2 and 3
>> and have the list to start the ball rolling, let's have at it ...
>
>I understand you: you have put work into this and want it get
>applied. The current situation is that you work by yourself and than
>show a big amount of patches which will lead to a long discussion and
>some part of your work will be rejected. 

Actually the corecleanup stuff has had its 7th version go out, and quite a
bit of it has been rejected, changed or merged/added to by others. Most of 
the recent reject comments though are about things that aren't in the patch, 
shown to be less than thoroughly thought out, or for non-technical issues 
where one can disagree endlessly just for the sake of disagreeing.

I don't think feelings of rejection is as much the problem as the petty 
delays in moving things along :-).

>IMHO one solution to prevent
>this in the future is to make smaller steps. Any maybe also
>concentrating on one patch at a time. You gain immediate
>feedback. 

You seem to want to spread this out over as long a period as you can, why?

Let's just fix these issues, and move on to better things. The stuff is
there, add it, change it and add it or drop the unimportant parts, but 
get to the stage where the critical functionality is in the codebase where 
it is needed, can be used and improved and dealt with from now on in nice 
tiny patches. 

>Currently I'm not happy with
>the arrangement, constitution and order of the bits.
>
>You should have an idea what bits I want. 

There is too much "I", too much "want", too much "guessing" too little 
proper technical review, and I think not enough consideration of what 
the Freeciv codebase needs or the various contributers are actually offering 
in some of these comments. But I suspect you are working too hard, as
opposed to getting others to work for you, and slowing down a little
on some things might help put others back in balance.

You did state earlier what your priorities are. I would hate to see these
changing. Less personal, less style, and more effort on the correctness
aspects would I think help, and cut down on the workload too.

The general community can do a lot of the review if you feed them questions
or ask for review comments on specific issues that you are unsure of. Send
an email with [REVIEW REQUESTED] or something to make it clear. It helps
to give a few examples or leading questions, as opposed to a general please 
look, though.

If something is really controversial or wrong, the list will take care of 
both the patch and the patcher.

Use the community to help you, and don't fall into the position of getting
overloaded and treating them or the contributions as adversaries and things 
to hold at bay :-).

>       Raimar
>
>-- 
> email: rf13@xxxxxxxxxxxxxxxxx
> "Despite all the medical advances of the 20th century, the mortality 
>  rate remains unchanged at 1 death per person."

Cheers,
RossW