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

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

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

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

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.

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.

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.

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.

Cheers,
RossW