[Freeciv-Dev] Re: Core code cleanups

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

At 11:04 PM 01/08/01 -0400, Jason Dorje Short wrote:
>"Ross W. Wetmore" wrote:
>> 
>> 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.
>
>I seem to have duplicated a lot of this work :-( in converting the code
>to entirely use the DIR_D[XY]2 directions with the adcj_dir_iterate
>macro.

I know, or at least I figured we were working in parallel. Which is why
I wanted to pass things along ... but it is really not duplication. This
is tricky enough that one person should not be doing it alone. The more
scrutiny it gets the better it is and doing it is the best way to scrutinize
the issues.

I've had the core macros for a number of months now, waiting for 1.12.0
to pass so they could be put out for review and into the codebase.

>Obviously nothing's going to be done with this before 1.12.0.  I'll try
>to merge our work, and see what comes of it.

Yup, I'm willing to help if you need it.

>
>--- Technical details ---
>
>Most or all of the work I did was trivial, I remember only about three
>tricky parts:
>
>1.  128>>dir is intended as 1<<reverse_dir(dir).

Yes ... if you recognize that DIR_DX2 direction ordering is different.
Your reverse_dir and my DIR_BITR should be identical code. So we just
get to squabble over the naming :-).

Note, lower case should be reserved for functions and variables, uppercase
for macros if that is the way they are created. But if it is dual use
in that there is both a function and a macro use the function form.

I have started prefixing with DIR_ for this set of objects. If you have
a consistent system as good or better, I'm easy.

>2.  There are some loops that have alternate code for non-normal cases,
>which won't work under the macro.  These were inevitably one-liners, and
>could be solved by pre-setting the values using memcpy, memset, or a
>simple for loop.

Yes, I exported the void_tile object and filled it in with the defaults
which are memcopyed in in various places before the loop. There were one
or two routines with their own definitions for the defaults that are now
using the same initialized values.

I also removed the 255 magic numbers and spread MAXCOST everywhere.

>3.  A very few places in the code use specific values.  Here the best
>solution IMO is to convert to an enumeration ASAP - then changing the
>enumeration will cause these to fall into place.  In particular, the
>dir_ok function uses specific values (and can also be written much
>shorter using the DIR_DX2 directions), and the client has several checks
>for (dir == NN) that are special cases for tile drawing.

If you mean the static or stack char arrays in a number of places used
to determine if something was Cartesian, the DIR_ISCART macro replaces 
that, and the arrays are all vanished and log comments taught to use
DIR_dn which is carefully grouped with all the rest of hte DIR_ stuff.

The manipulation to get the main directions is in straightest_direction
only now, and dir_ok does a trivial arithmetic to test for the back 3.
But this code is still DIR_DX2 specific. I didn't spend long enough to
make it generic, though the translation at the end  is probably where
this need to go, i.e. tx[].

>Of course, many of the loops are adjusting the same data and so must be
>converted to the "new" directional system together to work.  However,
>everything does seem to work...

I think we have hit most of the same fixes. gotohand and aiunit were the
two main places that "understood" directions and "opposite" directions
in linked ways. Most of the rest just need to have a consistent way of
doing things.

But there are GUI datafiles that are built in DX1 order still left in the
client and civworld areas, along with the code to build tiles based on
the map matching sides, I think. These appear to be self-contained, and
not interact with the core idea of iteration order as far as I can tell.
I wouldn't change them without getting a handle on just what this might
mean.

As far as I am aware, iterate macros should work fine in any core loops
that don't currently have them. I switched a few in passing. I have also
run enough auto games to be reasonably confident that nothing major is
broken. But this doesn't mean that some units might be pursuing suboptimal
strategies :-).

>jason short

Cheers,
RossW