[Freeciv-Dev] Re: [PATCH] map_adjust_* patch

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, Sep 01, 2001 at 02:33:15PM -0400, Ross W. Wetmore wrote:
> At 06:09 AM 01/09/01 -0400, Jason Dorje Short wrote:
> >Raimar Falke wrote:
> [...]
> >> > Also it would be nice if you could convert the places that you
> >> > touched to use map_inx() where appropriate.
> >> 
> >> Nice but not necessary.

I'm not sure if I understand this: are you working on a patch (based
upon/similar to Jason's) or are you planning to do one. You use past
and also "would". I would assume you have made these changes in your
working tree. Will you sent a patch? Should I apply Jason's?

> I've converted it to map_index() everwhere as requested by Raimar. I
> added a map_index_checked() in a couple places where it wasn't safe
> to assume normalization was done. This uses _map_adjust_* for the 
> moment but the usage is restricted to about a 30 line chunk of map.h
> where these things are currently defined.
> 
> >This patch is *only* replacing map_adjust_* with normalize_map_pos. 
> 
> I cleaned out all map_adjust_*() usage everywhere including civworld 
> except for the couple macros in map.h that use _map_adjust_* as utility 
> macros. normalize_map_pos updates reference variables and thus it can't 
> always be used in macro context. If functions are the final form these
> last dregs can go.
> 
> >I've tried to avoid (for now) substitution in code that needs a lot of
> >cleanup so as to avoid conflict with other patches (particularly
> >Ross's).
> 
> I wouldn't worry about this. The idea of not touching patches so sets
> can be serially applied is not yet appreciated in the current development
> context. 
> 
> If code reviews were more comprehensive, and agreement was reached on 
> patch scope and details by the end of the first review phase, then a lot 
> of the petty micro-management suggestions that are so irritating, and a 
> lot of the urge to do cosmetic reformats would hopefully die out. The 
> second or if there is some serious problem third patch should always be 
> applied pretty much untouched if accepted, and any lingering minor issues 
> dealt with in subsequent bugfixes. It is not like the current record of 
> patch applications is always of pristine release quality, or the bar isn't
> pretty low in some cases :-).

Yes I made some mistakes in the past. Humans make errors and there was
not much testing besides me.

> 
> >jason
> 
> RANGE_CHECK and RANGE_CHECK_0 are also implemented.
> 
> I split out the profiling macros in map.h from the topology so they don't
> need to go into any patches. This adds a 25% hit to the runtimes, but such
> things can done later once everyone figures out and comes to agreement on
> what the final flavour is to be.

What profiling macros?

> There are some minor changes to mapgen to remove some of the hardcoded
> assumptions about topologies, and do a slighly better selection of the
> starting locations.

This would be an extension to Jason's patch.

> And I updated civworld to bring it into line with everything including 
> tilespec.[ch]
> 
> I believe this covers most of Raimar's current requirements.
> =====
> 
> > We've had this argument for so long that I don't think anyone really
> > cares about it anymore, but...at least lets be consistent with the
> > naming.  If we can't agree on anything else, let's go with "valid"
> > (instead of what is currently "real") and "proper" (instead of what is
> > currently "normal").  But lets change them all at once, and keep them
> > the same throughout.
> 
> I vote for "real" and "normal" with the obvious meanings ;)
>       Raimar
> Me too. since this is the current code usage and will mean fewer changes.
>        Ross
> 
> IMHO MAPSTEP should be a method which does:
> int mapstep(int old_x,int old_y,int *new_x,int *new_y,int dir)
> {
>       *new_x = old_x + DIR_DX[dir];
>       *new_y = old_y + DIR_DY[dir];
>       return normalize_map_pos(new_x,new_y);
> }
> With such a method we get everything which has to do with position
> creating and checking in one method and place. The method resembles
> common code structures.
>       Raimar
> 
> Yes. The key elements are that MAP_STEP should return a TRUE/FALSE value,
> and probably fillin the new coordinate positions even if they are unreal.
> If done as a macro, it should be an expression and not a code block. Thus
> it can be used in "if" statements as the guard condition.
> 
> As a minor nit, I would also put the two return arguments at the beginning
> (or end), and group the read-only ones in a single block as in
>   int mapstep(int *new_x,int *new_y, int old_x,int old_y,int dir)
> This is a pattern used in most of the *_iterate macros, and thus if
> these are different, will cause a lot of unnecesary bugs.

Fine with me.

> Unfortunately, MAP_STEP still suffers the same limitations as 
> normalize_map_pos in requiring return reference locations. And it isn't the
> complete solution unless dir is expanded to include arbitraty steps, at
> least in a reasonable local context. So it is likely that one will still
> need an explicit normalize_map_pos() in some cases for awhile.

We will see how many will remain.

> But such things will consolidate a lot of code into a few key blocks in
> map.[ch] which will make any future extensions a lot easier, plus make
> the current code a lot more standardized and hence robust. So doing 
> even a small part is good.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Only one human captain has ever survived battle with the Minbari
  fleet. He is behind me. You are in front of me. If you value your 
  lives, be somewhere else."
    -- Ambassador Delenn, "Severed Dreams," Babylon 5