[Freeciv-Dev] Re: [PATCH] Map cleanups (PR#1208)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Jan 07, 2002 at 12:31:10AM -0500, Ross W. Wetmore wrote:
> At 03:45 PM 02/01/06 +0100, Raimar Falke wrote:
> >On Sun, Jan 06, 2002 at 04:32:02AM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> >> Ross W. Wetmore wrote:
> >> 
> >> > This series of patches cleans up a number of cosmetic and real issues
> >> > in map.c and map.h. It is split into a number of sub-patches for the 
>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> > convenience of reviewers, but is intended as a single submission, and
>      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> > only tested in a fully installed state.
> 
> Just to reinforece the previous warning ...
> 
> You may need to be careful with some comments. Some things are present
> in the subpatches to make things obvious, but go away in the full patch.
> 
> I could submit this as a single patch, but that tends to make people
> crotchety. GB once asked how to get comments into a diff - think about
> this as one possibility.
> 
> Certainly if these are not batched into CVS, i.e. applied to a sandbox
> verified, then checked in at one time there will be problems. I will 
> then need to go back to submitting single patches to prevent bugs in 
> my submission from such activities as has occurred in the past :-).
> 
> >> > 0)  mapclean_00.dif
> >> >     This is a pure cut and paste line rearrangement of the existing
> >> > files intended to improve the grouping of related functionality and
> >> > simplify/localize the subsequent changes. The line and character
> >> > count (wc) should be unchanged. One can repeat this by doing cut and 
> >> > paste operations on a clean CVS file to match side-by-side views with 
> >> > the patch result, and use diff as a final verify step if you are
> >> > consciencious or lacking in trust :-).
> >> 
> >> I trust you, and everything looks sane here.
> >> 
> >> There are advantages and disadvantages to this change. 
> 
> The rearrangement is the accumulated result of all the smaller changes
> it is not really a "change" in itself. But it can be dealt with as a 
> more global discussion provided you don't attach any more meaning to
> this than it is the result of ... and not an intended goal in itself.
> 
> > In the long run, 
> >> only the advantages matter.  As more and more functions have been 
> >> added,there has been some consistency in their placement but no 
> >> overriding organization.  Re-ordering them to match a completely 
> >> consistent theme will certainly make future coding easier.
> 
> I assume a carefully designed rearrangement is an out-of-scope suggestion, 
> and something to be dealt with as a separate task from merging corecleanup
> fixes. Some minor adjustments are of course reasonable of consideration.
> But a new key requirement for this patch is IMHO the sort of unreasonable
> thing that tends to cause a lot of friction and delay of completed work.
> 
> As background considerations that were underlying rationale though ...
> 
> As people, like maintainers change things they have a tendency to delete 
> the original and put the new function at the bottom. This doesn't always
> make things the best over time, but here some of the clumping is a result
> of collecting sets of functions most of which are diddled or would move
> for some similar reason to reduce some of this scrambling. 
> 
> As one example, the ones that are potential macro candidates are put at 
> the bottom in a meaningful defined order.
> 
> In the header file, macros are almost *always* best pushed to the bottom.
> This has the advantage of keeping the static enums and defines or useful
> reference material near the top, while the macro code can run on forever.
> You can also get the function definitions out of the way, then replace
> them with macros. #undef'ing will restore the function in any compile
> hierarchy that specifically wants it.
> 
> Most of the header function reordering is from unscrambling the current
> mess of mixed macros, supporting subsystems and function code, with some
> attempt to keep things in the header grouped as in the "C" file when they
> were moved. The corecleanups are also a slightly older branch point on
> some things so there are changes being resolved from this realignment as
> well.

Initiate a style guide questionnaire about the order in the files and
see what happens.

> >> On the other hand, this will make many existing patches to these files 
> >> (map.[ch]) fail to apply.  In most cases this should be easily fixable. 
> >>   It is in any case a short-term problem.
> 
> Almost any patch of more than trivial size has a pretty high probability 
> of this :-). 
> 
> It is an interesting comment, but has it any relevance here, or are you 
> advocating nothing be submitted to CVS that might overlap with other 
> changes that may be in parallel development? 
> 
> This would be a bad policy to adopt or the general comment even be something 
> to think about except in some very special cases, i.e. two large parallel 
> sub-projects scheduling their final checkins.
> 
> If this is a new criteria for submission, we should probably make this a
> separate discussion topic and thrash out the rules in a separate thread.

I don't see this problem as big as Jason.

> >> Summary: assuming you agree with the organization Ross introduces, this 
> >> should be a beneficial change.  And if it's going to be applied, the 
> >> sooner the better.
> 
> If you want to do a reorg for itself, there are many other groupings that
> might be improved other than those touched here, or ones connected to 
> them. 
> 
> This I presume to be useful discussion but out-of-scope for this merge.
> 
> <ASIDE>
> Note, I am generally quite happy to see things move incrementally to a
> given goal over time. Thus I don't mind if "some" style elements from
> preexisting code aren't changed, or code uses the surrounding style
> even though not the standard for small fixes. Local consistency (at
> the function level) is better than none, and over time things will
> aways move to the norm so a big onetime fight isn't particularly 
> useful or necessary.
> 
> In the case of a more general structuring of files and code blocks,
> I think it would be useful to keep raising such elements and achieving
> a concensus and consistency with smaller changes that people are willing
> to make over time than trying to achieve the one-true-way all at once.
> </ASIDE>
> 
> >> I do disagree with some of the organization.  It takes code that is 
> >> topology-dependent and is currently (mostly) clumped together and 
> >> separates it all out.  Although is_normal / is_real / normalize / 
> >> nearest_real_map_pos are all placed together, regular_is_normal and 
> >> is_border have been separated from them, as has map_distance_vector. 
> 
> Actually, map_distance_vector() and related functions that use it are
> the group just above is_real_tile() and its coordinate handling friends.
> Which is where it was before ... I don't understand this.
> 
> The callers are below, because even with proper header definitions, it 
> is still better to define things before use, than vice versa.
> 
> If you provide a specific list of things you think are grouped and the
> criteria for that I will try to accommodate.
> 
> Until then I presume there is no tangible change here.
> 

> >> I 
> >> do think these should eventually all go into a separate file 
> >> topology.[ch], and if there is some agreement on this I'd provide a 
> >> separate patch to accomplish this.  So it shouldn't stand in the way of 
> >> inclusion.
> >
> >I think that it is hard to come up with a good (everybody agree to it)
> >organization. Also a seperation like map_topology.[ch],
> >map_tiles.[ch], map_part3.[ch],... may be better. The borders are just
> >more clear this way. So I think it should be applied but there may be
> >other opinions.
> 
> A clumping and any discussion of what is related to what, is a not 
> unreasonable first step to some future split. Certainly if a file has
> obvious subsections, gets big enough to be unweildy, or parts need to
> be specialized for different architectures this is relatively easy
> once the patterns and fault lines are established.
> 
> It is probably premature to "make" a split without some of this thought
> and discussion of long term goals though.
> 
> Once there is a reasonable concensus to what sort of things will be
> accepted, then someone should be given the responsibility and latitude
> to just go and do it, and the fights that have been fought and decided
> should not be allowed a rehash unless there is some new issue, or a 
> real abuse of privilege.

I thing such a discussion or at least the result of our mind has to
documented even if you move functions around in a file. The more
precise the definitions of the sections are the better.

> >> > 1)  mapclean_01.dif
> >> >     This is a series of local cosmetic and minor updates which should
> >> > have no global scope or significant impact on game execution. They can
> >> > largely be scanned in a superficial way, i.e. no heavy thinking.
> >> 
> >> For the most part, yes.  But you've slipped some comments in that do 
> >> require thought.
> >> 
> >> This patch seems to consist of the following:
> 
> There is only one patch :-)
>  
> >> * A lot of places that have been changed only in spacing, i.e. to match 
> >> freeciv's indentation rules.  Although purely formatting changes are 
> >> frowned upon, most of it is pretty innocuous stuff (i.e. nothing 
> >> controversial), and the rest is stuff that indent would probably mangle 
> >> (by splitting lines at the wrong place).
> >> 
> >> You do have
> >>    -  c=map_get_terrain(x,y);
> >>    +  c = map_get_terrain(x,y);
> >> in which the spacing on the second line should be fixed.
> 
> Sure.
> 
> As a note, I'm not style-police minded, but tend to do things for
> readability or maintainability under the enough but not too much
> policy :-).
> 
> I can easily get carried away once I get into standardizing though.
> 
> >> * A lot of places that replace map_get_tile(x, y) with MAP_TILE(x, y). 
> >> This is faster and just as correct.  Some of them are in one-line if() 
> >> statements which have not been separated out; Raimar may want them 
> >> separated (although I imagine other maintainers won't care).  
> 
> There was an earlier partial update. The presumption here is that 
> completing it for consistency within the file is not unreasonable.
> 
> >> It would be better IMO to get rid of MAP_TILE altogether and make
> >> map_get_tile a macro, but that's easily done later.
> >
> >Ack.
> 
> It is trivial to do this. In fact the corecleanups had these sort of
> global macros but the discussion at the time was not in favour of the
> sort of widespread use.
> 
> Note, all one needs to do is add the appropriate map.h #define and
> rename within this file in the same way that is_real_tile() or 
> normalize_map_pos() are handled.
> 
> The local flavour of this was I thought part of the original decision.
> Have the winds changed?
> 
> >> * 1 re-ordering of an if() statement.  This makes it both easier to read 
> >> (IMO) and faster.
> 
> FYI: it is because in the corecleanups this is more complex, the change
> is a merge with fixups for the fact that the whole process is being
> split into bite sized pieces and redone several times as features are
> added in. But in the corecleanup changes, order did play a role :-).
> 
> There will be a one line change to the separate pole handling when the
> topology is extended to WRAP_Y cases (only one polar continent).
> 
> >> * You've added comments in some places.  This can only be a good thing, 
> >> so long as they tell the truth.  In other places you've changed 
> >> comments, which may be more questionable.  See below.
> >
> >> * You have moved the call to reset_move_costs down to the bottom of 
> >> change_terrain().  Although this makes sense logically, I can't directly 
> >> answer to its correctness.
> >
> >It is ok since S_MINE, S_FARMLAND nor S_IRRIGATION change the move
> >costs.
> 
> It is one of those maintenance things. There are a lot of other changes
> that probably need to go here. If they and or the intervening code 
> actually did modify something then the reset_move_costs needs to be run 
> after to make sure it reflects the valid state of the game.
> 
> This actually has no effect, but the reason is very real and the current
> position actually a mistake, so it is a fix and not a convenience :-).
> 

> >> * You've changed several lines of the form "return foo;" to "return 
> >> (foo);".  I can't say I agree with this change; any of my patches would 
> >> most likely just change it back :-).  Perhaps this is a question for 
> >> another style vote?  (Or did we already vote on it and I missed it?)
> >
> >It is definitely a style question. I'm also not sure if we have
> >already covered it or not. I still have to apply the results of the
> >first part.
> 
> There has been a flipflop of such things from the original branch point.
> But in cases where this is a deliberate change you will probably find that
> it brackets a boolean expression, and not just a return value.

Ok.

> Compilers give a lot of warnings about possible boolean bracket additions.
> It helps to keep things straight even if not needed. Again it has 
> maintenance vs purely subjective style aspects and is not a personal
> preference so much as a general industry tilt.

Another style question.

> >> * Some of your changed/added comments are very questionable to me.
> >> 
> >> The following added lines in the function header comment for 
> >> map_distance_vector:
> >>    +sq_map_distance is defined to be the square or the trigonometric 
> >> distance.
> >>    +
> >>    +map_distance is the city grid distance, i.e. no diagonal moves allowed.
> >> need to be improved on.  "sq_map_distance is the square of the 
> >> trigonometric or Pythagorean distance, and the sum of the squares of the 
> >> distances in the x and y direction", while "map_distance is the grid or 
> >> 'Manhattan' distance, and the sum of the distances in the x and y 
> >> direction".
> 
> Ok, added Pythagorean and Manhattan to the mix.
>  

> >> Then we have the following comments for normalize_map_pos:
> >>    +FIXME: This should test for realness using is_real_tile() before 
> >> touching
> >>    +       coordinates and update only if needed, thus avoiding side 
> >> effects.
> 
> >> normalize_map_pos() has _inherent_ side effects, and trying to pretend 
> >> otherwise is foolishness IMO.
> 
> Clearly you haven't looked at the (whole) patch. Side effects have been 
> removed from the current CVS bringing them into line with the original
> corecleanup. "_inherent_" is another of those mental religious terms
> not founded in reality here :-).
> 
> >>  As I've said before, there are three 
> >> things we could legitimately do to unreal coordinates: (1) leave them as 
> >> they are, (2) wrap them linearly as if they were real (the equivalent of 
> >> Gaute's wrap_map_pos), or (3) find the nearest_normal_map_pos().  #1 is 
> >> my preference and yours, but defining things this way would be bad.  I 
> >> would write this change like so:
> 
> void_tile-ism is the philosophy of converting bogus values to pseudo-real
> values so the local code does not break. The breakage is deferred to much
> later in the code where the original culprit can not be identified and
> thus held to blame. It therefore spawns a fatalistic philosophy wrt dealing
> with error conditions as they occur :-).
> 
> void_tile-ism is also the sibling of printf error checking and the disdain
> for dealing with error conditions in general.
> 
> It has been sufficiently blacklisted that clearly 2) and 3) can hardly
> be considered the "optimal" solution for the primary interface.
> 
> These are even worse than void_tile in that the returned values are
> completely indistinguishable from real values. void_tile was at least
> a recognizable reference.
> 
> If you do not build this into the normalize_map_pos() interface, and it
> is trivial and cost-free to guarantee it's adherence, then there will be
> numerous constructs of the form 
>   if( is_real_tile() && normalize_map_pos(&x,&y) )
>   if( am_i_sane() && normalize_map_pos(&x,&y) )
>   save_values(x,y);
>   if( !normalize_map_pos() && restore_values() )
> plus all the places that don't do anything and make the wrong assumptions.
> There are some already.
> 
> Unix errno is a good model here. It is set only when an error is detected
> and not touched otherwise. This means that successful returning functions
> do not trash an errno condition notification just because they were called
> during the handling.
> 
> Initially, I was ambivalent about this part of the interface, partly
> because there might be a compare operation or two saved if you only
> needed to test as you went along. But I have seen places in the code 
> where this causes even more effort to work around or recover from.
> 
> Making this a defined part of the interface does not break existing code
> and fixes a lot more, including that yet to be written.
> 
> I don't consider there to be any real grounds for either alternative.

This whole is discussion can be finished if is_real_tile is
implemented in terms of normalize_map_pos.

> >> /**************************************************************************
> >>   Attempt to normalize the map position (to wrap it into the set of
> >>   "normal" positions.
> >>     - If the position is real, the function returns TRUE and (*x, *y) are
> >>       changed to be the equivalent normal position.
> >>     - If the position is not real, the function returns FALSE and
> >>       (*x, *y) become undefined.
> >> **************************************************************************/
> >> int normalize_map_pos(int *x, int *y)
> >> {
> >>    /*
> >>     * FIXME: this should test for realness before touching coordinates
> >>     * and update only if needed, thus avoiding side effects.
> >>     */
> >> 
> >> You write comments of the form:
> >>    /*  ===============...===============  */
> >> and
> >>    /******************...******************/
> >> What's up with that?  Just stick to the second one.
> 
> I presume you refer to the section breaks in the map.h header file?
> 
> They aren't part of any specific comment, and the /***..***/ form is not
> applicable as it is a bracketting construct.
>  
> >> The following segment
> >>    +/* Map topology definitions and map dimension related macros */
> >>    +/* The default definition reflects a cylindrical map wrapping at 
> >> map.xsize  */
> >> should be one comment, not two.
> 
> I actually disagree though not strongly. 
> 
> I could add a space to make the subsection title stand out more from the 
> note that follows, or move it elsewhere as part of some larger block.
>  
> >> In the following comment
> >>   /*
> >> - * Sets (dest_x, dest_y) to the new map position or a real map
> >> - * position near. In most cases it is better to use MAPSTEP instead of
> >> - * SAFE_MAPSTEP.
> >> + * Sets (dest_x, dest_y) to the new map position or a nearby real
> position.
> >> + *
> >> + * In almost all cases it is better to use MAPSTEP and deal with the
> unreal
> >> + * coordinate case explicitly, instead of SAFE_MAPSTEP and the 
> >> void_tile-ish
> >> + * problems that produce surprises later.
> >>    */
> >
> >> although I agree with you in principle ("in almost all cases it is 
> >> better to use MAPSTEP"), the "void_tile-ish problems that produce 
> >> suprises later" part is completely misplaced.
> 
> Anyone who sees SAFE_MAPSTEP() and the original description is clearly
> going to make the wrong assumptions about the "safety" of using this.

Note that SAFE_MAPSTEP has changed yesterday. I'm also considering
moving SAFE_MAPSTEP to client/tilespec.c because I see no general
usage for it.

> >>  First of all, there will 
> >> be no void_tile-ish problems by definition, since unreal coordinates 
> >> will _never_ be returned by the macro.
> 
> And that is the crux of the void-tile mistake ... it doesn't identify
> unreal conditions and returns a completely bogus real result which is
> propagated until it causes a weird downstream problem.
> 
> For example, having a unit SAFE_MAPSTEP() until it runs out of movement
> points would result in an infinite loop if it encountered an unreal
> position.
> 
> >>  Secondly, this comment is aimed 
> >> at convincing the casual user to not use this macro, but it will only be 
> >> understood by someone who knows the history of the internals of map.c.
> 
> Any casual user that doen't understand the use of this shouldn't use it.
> Anything to convey this message in clear terms is IMHO only beneficial :-).
> 
> >Ack. void_tile is gone and shouldn't been mentioned anymore.
> 
> It is a pity the philosophy hasn't gone. Until it has, remembering past
> history can only help to avoid repeating past mistakes. I disagree quite
> strongly with your opinion and expressed desire to suppress others here :-).
> 
> There seems to be an awful lot of redefining everything from variable 
> names to concepts by those that seek to reimplement discredited notions 
> or just put their stamp on things - not a good sign and too much is
> being lost in the process. More history needs to be retained :-).
> 
> >> This comment
> >>    +/* This is a temporary measure to track down
> >>    +   non-normal cordinate use */
> >>     #define CHECK_MAP_POS(x,y) assert(is_normal_map_pos((x),(y)))
> >> seems quite wrong to me.  Although the current form of CHECK_MAP_POS may 
> >> be quite temporary, it's neither feasible nor desirable IMO to go back 
> >> to the old system of using normalize_map_pos everywhere and returning 
> >> void_tile if we ever end up with an unreal tile.  One possible change 
> >> would be something like
> 
> The old system used normalize_map_pos() and dealt with the unreal problem.
> The old-old system and the new system would return pseudo-real values and 
> just continue merrily along their way.
>  
> >> #ifdef DEBUG
> >> # define CHECK_MAP_POS(x,y) assert(is_normal_map_pos((x), (y)))
> >> #else
> >> # define CHECK_MAP_POS(x,y)               \
> >> #   if (!normalize_map_pos(&(x), &(y))) { \
> >> #     freelog(...);                       \
> >> #     abort();                            \
> >> #   }
> >> #endif
> 
> The use of normalize_map_pos() everywhere was decried for performance
> reasons, it *is* the robust solution. By adding back these checks you are
> forfeiting the performance benefit. In the discussion of the time it was 
> argued that eventually there would be a benefit, so this overhead vs the
> proper solution was only temporary. 
> 
> If you now think this needs to be permanent, then the solution is to return 
> to using normalize_map_pos() which you have done here. But in the original
> code it was often used properly in context which this has now lost in 
> favour of fairly widespread generic constructs like map_inx().
> 
> I do agree that something like this is a reasonable default for dealing
> with unreal conditions (note not unnormal ones) in a purely generic 
> context, but once at the head of a routine, rather than on every map
> or tile lookup was a much better solution.

The CHECK_MAP_POS calls will stand in the code for a long time but
will be disabled by default in the near future.

> >> but calling CHECK_MAP_POS "temporary" just seems like a misplaced 
> >> attempt to make it so :-).
> 
> That was your statement and therefore a condition on the decision that 
> was made at the time - just recording the history to make sure it doesn't
> manage to lose itself :-).
> 
> >> Summary: this patch is a collection of disjointed changes.  Although the 
> >> map_get_tile->MAP_TILE ones are good and the straight formatting changes 
> >> are decent, I have objections to a lot of the comments you've changed. 
> >> Another opinion is needed on this.
> 
> Yes, we agree on most of this :-). 
> 
> This patch is a general file cleanup and this subset of largely cosmetic and 
> noise-level bugs. 
> 
> I think the discussion items are things that do need to be resolved as
> they underlie a number of larger tasks that are bogging down :-).
> 
> >> > 2)  mapclean_02.dif
> >> >     This contains a small number of more significant changes plus an
> >> > extensive update of map.h non-directional macros and fixes outstanding
> >> > bugs with normalize_map_pos() and friends. While the functional versions
> >> > are both equivalent and fixed, most program use should now be via macro 
> >> > for increased performance. These features have been in the corecleanups 
> >> > for 6 months and are completely compatible with the current CVS codebase,
> >> > so there should be little or no concern about risk.
> >> 
> >> Ahh, now we get into real disagreement :-).
> 
> It is one patch ... 
> 
> >> > a)  map_city_radius_iterate -> city_map_checked_iterate
> >> >     This fixes unnormalized access to map_get_terrain which can no longer
> >> >     deal with real but non-normal coordinates.
> >> 
> >> As a side note, I don't think either of these names are particularly 
> >> good.  map_city?  checked?  huh?  I have to look the city macros up 
> >> every time I use them.
> 
> This should be largely out_of_scope. The names are in CVS for the most
> part.
> 
> If we open up the naming, then SAFE_MAPSTEP is one of the first I think 
> we should decide on. I'm in favour of a Raimar name
>   BOGUS_MAPSTEP_FOR_THOSE_THAT_LIKE_TO_GENERATE_RANDOM_SURPRISES()
> :-).
> 
> >So what is the long term goal? remove map_city_radius_iterate altogether?
> 
> Yes, when this cleanup merge is done, the followup patches to clean out a 
> number of the residual coordinate and iterator uses can be processed as 
> batches of smaller local fixes. That is why map_adjust is left, but marked.
> 
> As I say, if you want I can do it all at once. It is certainly much faster,
> less of a problem in duplication or side trips to generate the meta-stable 
> intermediate states, etc. Look at the corecleanup to see where it is going 
> in a close to final form or to try things out. This iterator does not exist 
> there.
> 
> Much of this changes with the topology fixes, since it is these few basic
> elements that form the valid interface, and the implementation here is where 
> the core topology changes will largely go. But first one needs to get the 
> interfaces in so the code can be updated to use them and there is something
> to build on.
> 
> Note, this is in the 5-10% range of easy things to get out of the way.

I agree that the goal is ok. However I had some problems with the
iterators of an old corecleanup version. So I have to take a look at
the current version.

> The only thing of contention here is the agreement to go and removal of the 
> standard CVS blockages and systemmic breakdown of the processes.

The change in mapclean_0* is ok. You have to make a proposal for the
further changes (patch or description).

> >> > b)  && -> || mistake
> >> >     One T_UNKNOWN state can be handled, two cannot, and none is excessive
> >> >     restriction on the server AI.
> >> 
> >> Gotta ask the AI people on this one...
> 
> It is good ... 
> 
> >> > c)  is_normal_map_pos() -> normalize_map_pos()
> >> >     Returned values must be normalized, so just do it and return.
> >> 
> >> 
> >>     do {
> >>       *x = myrand(map.xsize);
> >>       *y = myrand(map.ysize);
> >> -  } while (!is_normal_map_pos(*x, *y));
> >> +  } while (!normalize_map_pos(x, y));
> >>   }
> >> 
> >> This change is incorrect.  Consider an iso-rectangular cylinder.  In 
> >> this case, all positions will be real, and so will be accepted by 
> >> normalize_map_pos.  However, it is only normal positions that we are 
> >> interested in.  Changing the code like this will result in some 
> >> positions being picked more often than others.
> 
> Consider the iso-rectangular cylinder, it has a map.xsize x map.ysize
> native coordinate set where all values are treated exactly the same
> as the current standard rectangular cylinder.
> 
> But what you are missing is the native coordinate conversion because 
> that isn't in yet. Look at the prototype/corecleanup version.
>     do {
>       *x = myrand(map.xsize);
>       *y = myrand(map.ysize);
>       native_to_map_pos()&x,&y);
>     } while (!normalize_map_pos(x, y));
> which typically takes one pass per coordinate requested. Native 
> coordinates are by definition normal as they are currently in the 
> standard map. By normal one means no real coordinate appears twice.
> 
> For now, there is only standard == native coordinates, so this is correct.
> 
> This solution takes the tack of properly defining the random selection
> to use the native normal space. Other solutions do things in cruder
> more expeensive ways but hopefully not in Freeciv.
>  
> >> If you want to rewrite the function entirely to take topology into 
> >> account, that is a valid change.  (I'd wait to get a second topology 
> >> before doing this, however :-).)  Doing it like this is just asking for 
> >> trouble.
> 
> No, it is perfectly safe. Since in fact is_normal_map_pos() just calls 
> normalize_map_pos() and throws away the values, using an extra function 
> call in the process, this change is actually more efficient so it is 
> worth doing this part now.
> 

> >> > d)  is_real_tile() is a test, not a full normalization, make it so.
> >
> >This spreads out knowledge about the topology. Provide numbers which
> >show how much faster it gets this way.
> 
> Accept it. Numbers aren't needed for the obvious.
> 

> Clearly, the test will be more efficient than the function call to
> the full algorithm that includes the test.

I agree. However I don't think that you can measure the difference.

> This restores the original CVS behaviour that the corecleanups did not 
> change.
> 
> Calling a full normalization function to return is_real, vs separating
> the is_real test condition of normalize_map_pos() into a separate 
> element that can be called on its own or as part of the full process
> is just a better partitionning of the interface.
> 

> Or to follow your logic get rid of is_normal_map_pos() for the same reason, 
> that it spreads the topology knowledge too far. 

No. Why? It doesn't hold topology knowledge.

> Plus the fact that it only tests for the soft condition that doesn't
> matter, whereas is_real_map_pos() at leasts tests for the correct
> hard condition of unrealness.

It was constructed for this soft test. This isn't a weakness.

> >> > e)  normalize_map_pos() should test for realness before it undertakes
> >> >     any transformation. This fixes any potential bugs from side effects.
> >> 
> >> Heh.  The bug is not here, but you should fix it anyway :-).
> 
> Touche ...
> 
> But actually, there are GUI spots that had comments about not adjusting
> the values for unreal tile positions. The fix to map_adjust_*() at these
> points is normalize_map_pos() and this preserves that characteristic so
> workarounds aren't needed.
> 
> >> > f)  nearest_real_pos() is defined consistently with normalize_map_pos()
> >> >     to return FALSE for unreal coordinates and TRUE for real. The only
> >> >     difference is that it returns arbitrary but normal results in the 
> >> >     unreal case, i.e. controlled side effects.
> >
> >The changes are ok.
> >
> >> This is sensible.
> >
> >> Note one alternative would be to get rid of nearest_real_pos
> >> entirely and give this functionality to normalize_map_pos.
> >
> >Ack. However I have no idea how hard it is to convert the remaining 3
> >(three) cases.
> 
> void_tile-ism is so hard to stamp out ... it truly amazes me, this
> tenacity in its adherents.
> 
> The solution is of course to stomp out this function period, just like
> void_tile, map_adjust_y(), SAFE_MAPSTEP and the rest of the mutations.

You have my support. Except for SAFE_MAPSTEP (see above).

> >> I don't advocate doing this (it will _very_ easily lend itself to
> >> misuse), but it would allow us to eliminate one function.
> 
> I commend your first preference ... the latter mistake I waffled on at
> one point myself.
> 
> I view nearest_real_pos() as map_adjust_*() with unfortunately a longer
> lifetime because it is a newer mutation.
> 
> >> > g)  RANGE_CHECK macros are added and used to improve performance by
> >> >     reducing execution in most cases where applicable by a factor of two.
> >> 
> >> I find the whole RANGE_CHECK_0 thing pretty unreadable and worthless. 
> >> Even if such a thing were included in CVS, I'd still write
> >>    if (0 <= a && a < b)
> >> instead of
> >>    if (RANGE_CHECK_0(b, a))
> >> Although I haven't checked it, I find it hard to believe that any 
> >> worthwhile compiler couldn't optimize this.
> 
> All the ones I've looked at don't. 
> 
> The double test is an endemic operation in all the code, normalization
> for instance, it was worth a couple percent overall :-).
> 
> >This is a chicken-and-egg problem. Ross can't show the nubmers until
> >all code is converted. I won't accept it until I have some
> >numbers. Ross however won't start until he has the promise that it is
> >accepted.
> 
> From the mailing lists ...
> 
> <Ross>
> If there is agreement that RANGE_CHECK is the name and function that
> should be used in all BOUNDS_CHECK or MAP_CHECK operations I will make 
> the necessary changes and put out a patch for this. Do you want a simple 
> zero based change or the more complicated arbitraty integer range?
> 
> <Raimar>
> Can you make a patch with RANGE_CHECK and RANGE_CHECK_0 and the
> changes to is_valid_city_coords?

You got me. Since you have digged up the emails: had anybody mentioned
some numbers?

> This was at one time a decided issue that died along with other parts
> that weren't - the macro name is Raimar's I believe :-).
> 
> I don't see any reason to repopen the debate - we can find enough things
> to disagree on that will likely keep debate alive in useless wrangling
> and kill patch submissions for the forseeable future :-).
> 
> The two steps back before one makes even one forward is definitely an
> interesting process strategy though.
> 
> >> > h)  Wrap and clipping macros are added to standardize code use. Overtime
> >> >     all map code that does local flavours should be updated to use these.
> >
> >> > i)  map_adjust_x() and map_adjust_y() are redefined to be explicit
> >> >     wrap and clip operations for the standard cylindrical map, as the
> >> >     first instance of standardization :-).
> >> 
> >> This is very bad IMO.  Standardization requires that 
> >> map_adjust_x/map_adjust_y be removed entirely.  This change just adds a 
> >> lot of importance to their definitions - you take 7 lines of 
> >> #definitions that are currently only used in a very few places 
> >> (hopefully soon to be removed), and change them into 30 lines of 
> >> #definitions within #definitions with #definitions, plus extensive 
> >> comments explaining how they should be used (including saying that 
> >> they're about to be removed, fortunately :-).
> >
> >Ack. map_adjust_[xy] should die/definition moved out of
> >common/map.h. Move it (in its current form) to client/mapview_common.c
> >I will be happy.
> 
> I think we all agree these need to go (along with SAFE_MAPSTEP, ... :-).
> 
> map_adjust_*() goes as soon as the code doesn't need it, which may be 
> blocked on Andreas right now. 
> 
> It is gone in the corecleanups, so this is just one of those intermediate 
> inefficiencies because everyone want things done in small steps :-). 
> 
> Discouraging people from using it meanwhile is IMHO a good thing.
> 
> >> Also, the "void_tile-ish" comment here is in a similar position to the 
> >> one mentioned above.
> 
> Too bad you don't react as strongly to the philosophy or implementation
> of it as you do to the name :-).
> 
> >> > j)  map_to_index() is defined as the start of the map_inx() removal.
> >> >     When the corecleanup and topology fixes are all applied, this will
> >> >     be completely gone. index_to_map_pos() as the reverse operation is
> >> >     defined in the standard manner, i.e. returns updated coordinates
> >> >     and a status return indicating success or failure.
> >> 
> >> I definitely like the name change (although I would call it 
> >> map_pos_to_index).  I'm not sure what you mean by map_inx removal, but I 
> >> don't think I'm going to like it :-).
> 
> The leading "pos" was I believe debunked by others, (Tony, maybe?).
> 
> map_inx was long ago to be renamed, but it will live on and continue to
> multiply as map_to_index().
> 
> <ASIDE>
> There is a very standard and consistent naming in the corecleanups. When 
> the merge is complete hopefully CVS will have a similar standardization.
> 
> All these "coordinates" are basically interconvertible with the internal
> standard game coordinates. There are thus two functions to write for any
> new one, <blort>_to_map_pos() and map_to_<blort_pos>.
> 
> A key feature of this is that if something is easier done in a particular
> coordinate representation, you convert, do the operation and convert back.
> 
> For example, it gets rid of all the vector arithmetic in normalize_map_pos()
> and all but the special case part for mismatched axes in
> unnormalize_map_pos().
> </ASIDE>
> 
> >> > k)  Macro forms for is_real_tile() and family are defined for the
> >> >     cylindrical map. These will be upgraded to full topological forms
> >> >     when further capabilities (i.e. topological state) are added.
> >> 
> >> This is something Gaute has wanted to do for a while, and as you've 
> >> found the speedup is very significant.  It makes the code pretty 
> >> ugly/unreadable, but it's tough to argue with the results.
> 
> Luckily, it is broken down into small steps. Each step is not that
> difficult to work out. Treat the steps as atomic operations once you
> have understood them. The entire process is thus quite digestible and
> very true to the conceptual explanation.
> 
> >> > l)  The generic border tile macro is added and used in the current case.
> >> 
> >> Ugh.
> >> 
> >> Summary: I don't like most of these changes at all.  So we need another 
> >> opinion (aside from the is_normal->normalize change which must be fixed).
> 
> Reread the above counter. *must* is such a strong word for something
> that is technically very straightforward and provably correct, more
> efficient, etc.
> 
> We need to get past the religious debate sometime though (sigh).
> 
> >> > The changes have been sanity tested and client and server autogames run
> >> > for a combined 24 hour period without a problem.
> >> > 
> >> > In one case, savegames chksummed identically through end in 1885. 
> >> 
> >> I believe it.  I see nothing which could change the behavior.
> >> 
> >> 
> >> > Times were ...
> >> >   9:05  CVS-Jan04
> >> >   7:03  patched
> >> 
> >> Very impressive.  But a greater speedup can be achieved just by changing 
> >> is_real_tile, is_normal_map_pos, normalize_map_pos, AND map_get_tile 
> >> into macros - without the need for RANGE_CHECK or any of the other 
> >> implementation hacks you use.
> >> 
> >> In my test, I get the following times for an autogame (compiled with 
> >> DEBUG and -O2 and only run once, so maybe not a very good benchmark...):
> >> 
> >> CVS: 3m39s = 219 sec
> >> mapclean patch: 3m6s = 186s ~ 15% speedup
> >> simple macro patch: 2m36s = 156s ~ 28% speedup
> 
> In my books, 2 in 7 is about 28%. I'm not sure I understand the "greater" 
> comment. map_get_tile() is left out here. It is one of the key functions
> identified in the corecleanup section of map.h that used to be macro-ized.
> 
> BTW: the corecleanup runs at about half this speed on the same topology.
>      But it has iterator loop and other things that are going to be much
>      more controversial when they come out, even without the first 
>      exposure backlash, i.e. 6 month settling time.
>  
> >> savegames were identical for all three, so I can back you up on that.
> >
> >I feel not ready for another thread about performance, macros and
> >inline usage. So Ross please drop the RANGE_CHECK thing for now or
> >show some drastic numbers (with RANGE_CHECK only).
> 
> It is so nice to work with someone that is so flexible and consistently 
> willing to change his mind on things. It makes progress so much easier
> to deal with.
> 
> In summary, there are 2 minor fixes (will do in next pass), 4 major religious 
> wars and one skirmish to be fought over two comments and three functional 
> changes, and one previously agreed on and requested feature Raimar now wants 
> dropped.
> 
> Does this summarize the current status?

Besides the requested feature: yes it looks so.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "The very concept of PNP is a lovely dream that simply does not translate to
  reality. The confusion of manually doing stuff is nothing compared to the
  confusion of computers trying to do stuff and getting it wrong, which they
  gleefully do with great enthusiasm." 
    -- Jinx Tigr in the SDM