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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Aug 30, 2001 at 12:59:41AM -0400, Ross W. Wetmore wrote:
> 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.

I pointed out what I dislike about your current set of patches.

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

I have. I have also think I understood what you tried to do. But I
still don't know the reason. It is common to declare global variables
extern and define them later in some c file.

> >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 may be true for local city coordinates (0<=x<=4) but not for map
positions. Sorry if I confused 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.

Please explain.

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

But in the function body.

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

Map cleanups:
 - MAPSTEP as a separate patch (for the current state should also
 another method/macro MAPSTEP2 defined)
 - new method is_city_center
 - methods which transform local city position to map positions and
 back
 - create a method is_normal(ized)_position, insert an
 "assert(is_normal(ized)_position);" into every access
 method of map.[ch] (this can also be done depending on a preprocessor
 variable, a still better was is to make a preprocessor macro
 "ensure_normal(ized)_position" which is either the assert or a noop),
 debug, make a patch
 - remove map_adjust_x and map_adjust_y, debug, make a patch
 - come to an agreement what the final direction system should be,
 migrate, debug, make 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. 

Ack.

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

Can this be done before the part above? Because in your order we end
up with correct use of is_real_tile and normalize_map_pos but still
have some map_adjust_[xy] in the code. If the second is done before
the first all the the map_adjust_[xy] either go away or become
normalize_map_pos which afterwards has to be tested for proper use.

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

Ack.

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

I interpreted the "Alternate topologies require proper is_real_tile()
and normalize_map_pos() handling" as "sometime before working with a
position the position has to be checked for realness and should be
normalized". And yes the place where is this done doesn't matter. And
we both agree tha the best place are after the creation of the
position?!

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

07f contains a lot of stuff. Can't you just resent this extra asserts
and neccary code changes to other parts. From the Readme: "This patch
contains an extensive overhaul of map.c and map.h, though functionally
it has minimal impact to execution apart from DEBUG asserts added to
map_get/set_<ters>." Somewhere in the patch these assers are hidden
but as you stated it is a extensive overhaul". I won't apply such a
big change.

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

I won't apply 07f as a whole.

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

No. I just want to check what goes in. From the 07f for example I
can't see what you have done. Yes maybe the end result is fine but I
want to be able to track it. And the reordering of the file haven't
helped here. 

I tell you: sometimes I thought about doing it myself (I understand
you now Thue) the proper way [TM] but I resisted. If you have
something in mind it is difficult to change your expectations. This is
also the reason why I asked you to resent your changes in a rearranged
form. I'm sure you have some of the changes I want in your working
tree.

> 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

It is hard to bring your current state of mind across the wire. 

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

I see myself in the role as a manager and as a programmer. I think
have to channel efforts to get the project forward. (yes we are all
working in our spare time and personal itch and so on) I wasn't very
successful to get you on my path. My path of thinking isn't fixed and
isn't the only one (you may convince other maintainers to apply your
patch).

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

Style (as in tabs and spaces) is a non issue since (in retrospective)
I formated every patch anyway before committing it.

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

I'm sorry if such came attitude came across.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "The Internet is really just a series of bottlenecks 
  joined by high speed networks."
    -- Sam Wilson