[Freeciv-Dev] Re: [PATCH] Corecleanup_07Part1 has been put in incoming

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

"Ross W. Wetmore" wrote:

> This is the first part of corecleanup_07, resubmission of corecleanup_06a
> with applied fixes from review comments.
> 
> The rest will follow as soon as I catch my breath.
> 
> The following patches were produced as follows:
> 
> 1)  Changes to CVS Aug-23 were merged to rev the corecleanup_06 source.
> 2)  After the merge, each source was updated with any minor changes.
> 3)  The result ran autogames for 18 hours on at 8-9 games/hr w/o fail.
> 4)  Changes to CVS Aug-25 were merged to rev the source.
> 5)  After the merge, each source was updated with any minor changes.
> 6)  The result ran autogames for 4 hours on at 8-9 games/hr w/o fail.
> 7)  Patches were made by diffing against the Aug-25 source.
> 8)  These final patches were tested by applying them in sequence to an
>     Aug-25 snapshot, with build and sanity test run at each step.
> 
> -rw-r--r--  1 rww   users   4053 Aug 25 18:30 corecleanup_07.ReadMe
> -rw-r--r--  1 rww   users   8034 Aug 25 18:14 corecleanup_07a
> -rw-r--r--  1 rww   users  26698 Aug 25 14:16 corecleanup_07b
> -rw-r--r--  1 rww   users  11438 Aug 25 14:13 corecleanup_07c
> -rw-r--r--  1 rww   users  16027 Aug 25 14:17 corecleanup_07d
> -rw-r--r--  1 rww   users  12909 Aug 25 14:16 corecleanup_07e

Great work!

First of all, the buggy behavior I had before with the AI was due to a
bad patch update.  Ross easily spotted my error from the patch I was
using, so that's not an issue anymore.

I've looked over corecleanup_07a:

First and foremost, you forgot to change adjc_iterate_dir_end to
adjc_dir_iterate end in server/gotohand.c.  (Background: when Gaute
applied this macro, it was with the wrong name adjc_iterate_dir, and the
macro was only used in one place in gotohand.c.  Shortly thereafter, the
macro name was fixed to adjc_dir_iterate but the closure remained
adjc_iterate_dir.  It's an easy fix, and I'd almost consider it separate
from the others even though it's only two lines.)

You've gone out of your way to change the formatting Raimar used in
dir-patch1 (the IS_CARDINAL_DIR lines).  You've replaced his formatting
with my original formatting.

Your comment
+  /* TODO: if true, the code below breaks badly. goto_array_index was 0
+     and the waypoint had our current position, which doesn't seem too
+     unreasonable. This fix seems to work, but ...
+   */
needs closer inspection.  Unlike other cleanup fixes (replacing loops
with adjc_iterate macros), this is a real bugfix.

Your comment
+  /* FIXME: The GUI uses DIR_DX directions */
isn't clear because "DIR_DX" isn't well defined.  How about calling them
"vertical" directions (instead of rotational directions)?  That's what
you mean, right?  BTW, I'm still strongly opposed to having multiple
directional systems - I'd much rather take the time to make the GUI
system-independent.  (Right now almost everything uses DIR_DX/vertical
directions.  However, we've been talking about changing all of this to
use the rotational system.)  This patch doesn't separate the directional
systems...yet.

The following code still seems unnecessary to me.  You may argue that
it's "better", but I really don't see how it fits into this patch.  You
don't change anything else in the vicinity.
-  for (i = goto_array_index-1; i >= first_index; i--) {
+  for (i = goto_array_index; --i >= first_index; ) {

Wow, that's the end of the patch!  Good work breaking it up.

Two simulations confirm that the behavior is apparently sane.

With the above caveats, I recommend applying in the near future.

jason