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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Aug 26, 2001 at 04:38:58AM -0400, Jason Dorje Short wrote:
> "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.)

I'm working on the patch and also found this one.

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

Formatting is no problem.

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

I removed the comments because it is clear that something which does
"int x1=x+DIR_DX[dir];" is using DIR_DX directions.

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

I would also appreciate one system.

> 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; ) {

I also omitted this.

> Wow, that's the end of the patch!  Good work breaking it up.
> 
> Two simulations confirm that the behavior is apparently sane.

IMHO simulations aren't necessary for this kind of changes.

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

Almost done.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "USENET is *not* the non-clickable part of WWW!"