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

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

At 04:38 AM 01/08/26 -0400, Jason Dorje Short wrote:
>"Ross W. Wetmore" wrote:
[...]
>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.)

rww@wetrec[277] grep iterate_dir Patches/core*07?
Patches/corecleanup_07a:-#define adjc_iterate_dir_end
                       \
Patches/corecleanup_07b:-    } adjc_iterate_dir_end;
rww@wetrec[278] grep -r "_iterate_dir" .freeciv-Aug-25
.freeciv-Aug-25/common/map.h:#define adjc_iterate_dir_end
                           \
.freeciv-Aug-25/server/gotohand.c:    } adjc_iterate_dir_end;     

Are you sure, this is what I get with a quick check. And I don't see
how the source could have built after changing the header and not the
gotohand.c in 07b. 

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

I patched CVS changes into the previous submission. One ignores overlaps
that are already done, especially if there isn't an obvious real change.

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

There are other TODOs and "real" bugfixes too :-).

Thanks for highlighting this one. I tracked it aways and confirmed that
exitting here was safe, but didn't really understand why the failure
condition occurred. It looked reasonable to be in this state as far as
I could tell. If it is not, the exit fix should be removed when the 
original problem is found and cleared up.

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

Part2 is halfway done, and moving to DIR_DX2 exclusively in the core is 
one of the pieces, as well as the improved/consistent *_iterates.

We can argue how many blockages one should put in the way of doing this
when the patch is reviewed. IMHO, one should do things in well-defined
steps where possible, and adding all the GUI changes for amalgamating
the direction systems before getting a consistent system rather than 
after makes the size and risk a bit too much for my liking. Besides I
don't see a lot of people stepping up to do a GUIcleanup now that the
corecleanups are moving along.

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

You probably want to look at what the compiler does in the way of block
ordering and branching when you give it a hint that the test and decrement
are really a single step, as opposed to two things it has to keep separate.

It is a nit cleanup, and can be dropped if there is adamant misplaced zeal
for some sort of foolish form over content. :-)

But given the number of mindless cosmetic changes that the merge of CVS
patches turned up and had to be hand resolved, I don't understand why
one would object to one that makes a significant code change.

>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

You didn't sem to notice, but the client/control.c section of 07c is
missing. There are 2 square iterates in the previous 06a. I can send
the update piece or patch, but figured it would just fall into the
Part2 (sigh).

Everytime you do one of these there are going to be a few minor fixups
that slip though the validataion process though. 

Cheers,
RossW
=====