[Freeciv-Dev] Re: Corecleanup patch updates

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

I'll make a quick run through this to answer some of the questions,
and in some cases ask clarification, or present the alternate view :-).

Thanks, for taking the time, not only to look at it, but for the
detailed response. It is much appreciated.

Cheers,
RossW
=====

At 08:48 AM 01/08/09 +0200, Gaute B Strokkenes wrote:
>On Tue, 07 Aug 2001, rwetmore@xxxxxxxxxxxx wrote:
>> I've dropped an update in ftp.freeciv.org incoming to last week's
>> corecleanup patch that fixes a few bugs and is made against
>> 1.12.0-beta-3 aka 1.11.10.
>
>I have some comments:
>
>1) This seems to have your ship movement and mapgen stuff in it.  That
>   should be in a separate patch.
Agreed. 

When things are at a stage where this is worth serious consideration
for CVS, I'll split things into smaller digestible pieces. For now, 
the mapgen stuff was one of the best testbeds for a lot of the macro 
changes, and new macros. And this stuff was not really mature enough
for a headrev patch. I'm also interested in learning what some of the
coding style and other imbedded assumptions are, or how far they can
be stretched before I submit a major patch.

>2) I'm still not sold on the DIR_D{X,Y}2 stuff.  What is the advantage
>   of using them, as opposed to the old ones?

First, there were two flavours in the original code, and the changes
took over and have fully expanded on the second flavour. The first
DIR_DX (in array sequence order) is now only really used in GUI code.
There it seems to have found its way into graphics data fragments. But
this does seem to be completely self contained and isolated.

A rotation order as used by DIR_DX2 allows one to do a lot more 
directional manipulations trivially. For example, moving forward,
right or left one step is a wrapped linear index, and quite tricky 
in DIR_DX where 5 comes after 3 and 4 is between 1 and 6. For core
code including all the AI heuristics, this is quite useful and the
performance and robustness from a KISS implementation can be 
significant.

If you look at the adjacent iteration macros, they all run from a single
core loop. The start, end and increment give you cartesian, diagonal,
knights (1 diagonal + 1 cartesian into the second ring) with essentially
no extra code to maintain or update to fix all iteration bugs.

By organizing the first few rings outward in a contiguous array, one 
can use the same system for fast square iterations including or not
including the centre tile, by just a single offset change in start.

It might be useful to extend it to a 3 tile radius for most of the 
common local coding loops, as it is fast and simple == robust compared
to a lot of the current flavours for these.

Using one system everywhere makes the code more compact, easier to
understand as there are common basic underlying rules, and a lot less
pain to maintain or extend. 

Note, once the basic corecleanups were in, the code changes to handle
4 different map topologies were all done in a few hundred lines in map.h.
And they worked with about 3-5 bug fixes, most of which caught missed
corecleanups.

>3) Formatting: use 2 spaces for indentation; including in
>   find_city_beach().
>
>4) Be aware that when you replace for loops with adjc_iterate you are
>   no longer looping over the centre tile.  This causes bugs in
>   e.g. ai_manage_barbarian_leader().

When this was done, I usually also removed a loop check that skipped
the centre square. If I used adjacent in a spot where square was still
required, I agree I added a bug. I'll double check them all.

In this case, can_unit_move_to_tile() always returns false for the
current tile (it only wants to move to adjacent tiles). I think this
means that the centre tile code is effectively optimized out.

This may not be desired of course ...

>5) Do not peek at the variables that are used to implement any of the
>   _iterate macros in the code that uses them since doing so defeats
>   the purpose of the excercise.  In other words, if you need to know
>   what direction you're going in you should use adjc_dir_iterate()
>   rather than adjc_iterate() and peek(_nn).

I disagree that this is the purpose of the exercise :-). 

I am also quite open to having the loop variable be considered as part 
of the macro interface, i.e. fair game. The primary reason I see for
defining it internally is that it is always needed, and there may be
times when the user doesn't use it and would be confused by having to
provide a required element with no obvious reason.

But it is trivial to set up two flavours of macro, and there is at 
least a conceptual problem of having the same loop variable used in 
nested cases. I'm not sure if compilers always get the scoping right,
as at least at one point, they collected all variable definitions and
treated them as if they were all defined once at the top of the file.

I'll make a second pass over this.

>6) What is DIR_BITR() ?  A more descriptive name would be good.
>
>7) FC__MAP_C is not necessary.  AFAIK there is no harm in declaring
>   them as extern in map.h, and then defining them in map.c.

Again I disagree. This time quite strongly. Splitting the definition
and allocation/default values, which in themselves are very useful 
documentation just makes finding things in the code more difficult. 
In this case all the parts are in one spot, no confusion results and 
the fact that you need to allocation the storage in map.c or params.c 
or wherever one chooses tommorrow is totally irrelevant and meaningless.

Coding practices that obscure what is going on or force people to hunt
unnecessarily are bad anti-patterns to follow. If I change the example
slightly, I'm sure I could get you to agree on this principle at
least :-).

>8) You dropped an assert(is_real_tile(x,y)) in find_route().

There is only one assert(0) at the end in 1.11.10, and it is still 
there. Did I miss something?

>9) I'm not optimistic about reading values from void_tile.  IIRC we
>   have had void_tile clobberation bugs in the past, and in the long
>   term we ought to get rid of it entirely.

I am beginning to recognize the nerves some of these things hit. But 
the problems are not in reading from void_tile, but the fact that it 
is (still) returned by get_map_tile as a (bad) fix for people that 
haven't checked their coordinates. And this is caused by not really
having a well defined set of coordinate checking tools available, or
those that were available being perverted for sleazy reasons.

BTW: most of the underlying problems with this are dimishing rapidly
and it should be the case that you can remove it from here without 
problems now. In fact I set the return value here to "NULL" to trap 
a lot of the access points, and don't have any problems currently.

Doing iterate loops means most accesses are checked implicitly before
the map_get_tile() anyway.

Getting rid of adjust_map_* in most of its invocations is also solving
a lot of this. Using this, one usually doesn't do proper error checking
and there were no end of fixups to "adjust" coordinates that were not
legitimately adjustable. The "y" truncation fix is a prime example.

>10) I don't think the use of adjc_iterate() in reset_move_costs() is
>    appropriate, since memcpy() is rather ugly.  I think it would be
>    better to leave as-is here.

If you look at Jason's latest flavour for these which I have adopted, 
the main body of the loop is a "if" clause. One can do an "} else {" 
and run an invalid clause within the loop if needed.

But doing a structure copy which is what the memcpy does, is perfectly
reasonable to initialize, and there is no need to manually set each
value to a possibly hardcoded magic number that is the current buggy
style. If one wants to change the initialized default, one should be
able to do this once where it is defined and have all the code that
uses it copy from there. void_tile seemed to be the place to do this,
i.e. where it was already done.

On the flip side, are you suggesting that all the alternate map border
conditions should be hardcoded into this loop, even though it would be 
just about the only place in the code that does this?

Note: if you do a "set maptype 3" in my current sandbox, this loop now
will correctly handle Donut-World wrap conditions, whereas the original
code would quite possibly fault on map_get_tile, or do something bad
to the void_tile address that it returned (if you were running lenient).

I'll send out a copy of the current patch as soon as I make a pass over
some of the other issues - this will hopefully reduce the noise and give
everyone a chance to see what I am talking about since this is fresh in 
my mind from unscrambling it, but not yet in yours :-).

>11) Don't be afraid to change strange and cruel formatting to be in
>    line with Freeciv's style (K&R, two spaces for indentation) if you
>    happen to be touching some code.  This is particularly relevant
>    for mapgen.c.  However, don't do it if you're not touching
>    anything in the vicinity, since that only obscures the important
>    portions of your patch.
>
>Can you make another patch that incorporates these suggestion?

Will work on it over the weekend as a (number of) patches to 1.12.0.

>-- 
>Big Gaute                               http://www.srcf.ucam.org/~gs234/
>Yow!  I'm having a quadraphonic sensation of two winos
> alone in a steel mill!

And thanks again for the feedback. It is only from differing views that
things really get scrutinized and the best solutions emerge. So not seeing
eye-to-eye on some things is not all that bad :-).