[Freeciv-Dev] Core code cleanups [Was: Profiling Civserver again]

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

Attached is the diff patch from Jul29 to my current playground that fixes
some of the issues raised below, and may get things over the first hump
if you are still gung-ho for tackling some of this. If we converge to a 
common patch base, I can pitch in as well as I have a number of things
partway along this path.

WARNING: this is high risk development code, and while it passes my sanity
         tests of running civworld, civserver through a number of
         auto-games and a quick client check, it needs to go in at headrev 
         i.e post 1.12.0 and not on a release variant branch.

I have folded some of the recently discussed map.c functions into a macro
context. These can be turned on or off, and used as initial templates for
more profiling experiments. see USE_MAP_FUNCTIONS in map.h and map.c.

I have made a stab at converting DIR_DX/DIR_DY to DIR_DX2/DIR_DY2 order 
in the core code, and replacing loops with *_iterate macros based on this.
There is a tight coupling between some of the warmap techniques in 
gotohand.c with dependencies in scattered code that means this is pretty 
much an all-or-nothing cleanup. 

There are aeveral areas in GUI code, for example "hires" changes use the 
DIR_DX mappings, which appear to be independent and self contained. I have
not touched these.

In most cases I have tried to replace local coding techniques with generic
macros, so if some of this needs to be reset or done again, one should be
able to fix it in a minmum number of spots. Look for map.h DIR_ISCART and
surrounding macros to see how the DIR_DX2 ordering is implemented, and 
corresponging spots in the patch for the local code changes.

I don't believe there are any serious uses or dependencies on DIR_DX left
in common, ai, server and with the GUI caveats client or civworld base. 

Since this is my playground, you also get the mapgen and river move patches
for free :-).

Cheers,
RossW

At 09:37 PM 01/07/26 -0400, Jason Dorje Short wrote:
>Trent Piepho wrote:
>> On Thu, 26 Jul 2001, Jason Dorje Short wrote:
[...]
>> > Yes, DIR_DX2 uses a more intelligent numbering system.  However, it will
>> > be difficult to upgrade to use this data instead.  It's too bad DIR_DX
>> > wasn't written like this from the start.
>> 
>> I'm not sure it would really be that hard.  They are a lot of places
that use
>> DIR_DX to iterate over tiles, but most of them never actually look at
the dir
>> variable.  You could order the tiles any way you want and it wouldn't
matter.

If you look closely through the accompanying diff, you will find that there
is an interesting mix of no effect through intricate recoding required. The
latter comes when there are hardcoded techniques used to manipulate the loop
direction index in a conceptual way.

This is why I tried to replace as many as I could with macros that can be 
set in one spot if the compass direction or ordering is changed.

>> There are a few places that do use the direction, but usually the value
>> doesn't matter, as long as it is consistent.  The only place the actual
value
>> matters is where the opposite direction is found by using 7-dir instead of
>> (dir+4)%8.  And a few places which have a text array of "N", "S", etc. that
>> direction is looked up in.  Those should be changed to a function call
anyway,
>> instead of identical arrays repeated in several functions.
>
>I'm pretty sure most or all of the loops that iterate over the direction
>use the value within the loop - otherwise they should have been replaced
>by adjc_iterate long since.  I do believe that fixing most of them would
>be easy, but there are an awful lot of them.

There are subtleties and side effects that are not immediately obvious,
and lots of changes to miss things and create a bug. But it isn't "hard".

BTW: there are also a few bug fixes in passing, where test runs showed 
things to be slightly fishy.

>> > superior to the current setup).  It might be possible to do better by
>> > making the cardinality an option to the macro, since I know there are
>> > some places in the code where it is handled like this.  I'm not sure how
>> > many places there are that are like that, though.

There weren't that many, mostly in gotohand.c, and a few in aihand.c. It 
might be easier to tweak things now that there is a single consistent
flavour to most local code as well as the various macros that now exist.

But while there are trivial cardinal variants of the macros, many of the
code paths are dual use, and it isn't trivial to turn generic tests into
a macro cardinality precondition that means you can run just one macro.

>Yes, that's exactly what I was thinking of.  Definitely better.  Note,
>though, that although it will work with either your style of border
>checks or the one I'm using now, the DIR_DX2 ordering of directions has
>to be used to make skipping non-cardinal directions easy.
>
>cartesian_adjascent_iterate (which needs to be renamed), adjc_iterate,
>and adjc_dir_iterate could all be mapped to this macro.

Done, at least all based on the same macro core_iteration ...

>Well, that's bad :-(  I'll try to buck the trend, and clean up some of
>this code.

Let me know what you think, or if you want to outline certain areas to
pursue in more detail.

>Patch will follow...
>
>jason

Cheers,
RossW

map_patch-cvs-Jul31.gz
Description: Binary data