[Freeciv-Dev] Re: review of corecleanup_06

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

Lots of comments, and explanations ...

At 01:26 AM 01/08/23 -0400, Jason Dorje Short wrote:
>It's actually my adaptation to current CVS I'm looking at...the two are
>very similar.
>
>1.  I do not believe the use of if (ptile && ptile != &void_tile) is
>necessary in many cases.  For instance, in aiunit.c/540 you use this
>check within an iterator macro.  The iterator macros
>(square_map_iterate, in this case) should guarantee their coordinates to
>be normal[ized].  If you want to keep the check, it should be made an
>assert().

Agreed. The square_iterate should handle it here, and it is likely just
a missed cleanup in moving to the square_iterate.

I did get a core in the unit_list_iterate from a NULL ptile. But I can't
see why in its current form. Can you peek deeper and figure out if there 
is something we are both missing before I chalk it up to earlier code bug.

>2.  IMO all of your /* The GUI uses DIR_DX directions */ comments should
>have a FIXME in there.  The directions shouldn't be hardcoded into the
>GUI!

I'm not a GUI person, so take what I say with a grain of salt. I tried to
track things down to find out why, and reached a point where I think the
GUI is reading in graphics tile information from disk data files in DIR_DX
order.

But the GUI also has DIR_DX2 order in tilespec.c at least, so it is 
schizoid, and nothing it does should be assumed lightly.

All this stuff is completely self-contained and separate from the core
server directions though so GUI cleanup and/or resolution can be done 
by people that know what they are doing at a later date.

The trick is not to cross pollinate the direction fixes and thus keep things
separate.

>3.  In client/goto.c what is the point of this ugly code change?
>
>-  for (i = goto_array_index-1; i >= first_index; i--) {
>+  for (i = goto_array_index; --i >= first_index; ) {

Actually it is beautiful :-). Loop branching and blocks are usually
better handled when you don't split the decrement and test steps. It
is an automatic correction for such poor code style as the original.

>4.  In server/gotohand.c/1190 you add a normalize_map_pos call without
>checking it's output.  Every time a normalization is done the realness
>should be verified also - either by checking the return value of
>normalize_map_pos() or with an assert(is_real_tile(...)).

No! But what you are saying is the safe bet if you aren't sure.

In this case, you just need to make sure the coordinates are normalized,
not that they are real - that is guaranted. The original comment is a clue.

But I can accept an assert as a paranoid measure at this point. It is
just that sometime they will all need to be taken out, as the clutter of
a lot of unnecssary checks will become excessive the way things are going.

>5.  The same is done countless times throughout the client.  I think
>once this patch is into CVS asserts should be tried out in the GUI code
>and we see what breaks; until then that's too much to deal with. [1]

No! Again! Understand the client - which I don't - but in this accept
the fact that GUI stuff can handle and should handle non-real coordinates.
They are drawn as "black" tiles.

There may be some cases which I haven't left comments on or got wrong.
But a blind application of server principle here is not reasonable.

>6.  In client/tilespec.c you're still using "magic" numbers with the
>DIR_DX2 system.  This is unfortunate but perhaps not fixable at this
>time.  A FIXME/TODO comment would be nice.  You've also left the
>topology hardcoded here - again a FIXME would be appropriate.

Yes. We need to reach concensus on the DIR_DX2 fix. This is the time to
update the current code constructs, not do it twice. 

corecleanup_06a doesn't deal with this on purpose.

>7.  I'm not happy with the naming of the city iterator macros. 
>Obviously city_map_iterate_checked should be city_map_checked_iterate,
>but even with that fix the names are unclear.  This is also outside the
>scope of this patch; city_map_checked_iterate should do for now.  An
>additional consideration: in many places city_map_checked_iterate
>replaces city_map_iterate_outward; hence the outward iteration is
>clearly a necessary part of city_map_checked_iterate although the name
>does not indicate that.

I'm a weathercock when it comes to which side of the iterate to suffix, 
and am not even consistent in referencing the existing ones without 
explicit checking. So, anyway suits me.

In this case is was city_map_iterate and city_map_iterate_checked and I
probably matched the longest string to indicate the association.

For checked in corecleanup_06c: map.h all the map_{g,s}*'s has this suffix
with the constant meaning. I use _unchecked for those that are dangerous
and programmers need to be alerted.

>8.  In server/cityturn.c this code also seems unhelpful.
>
>-        if (cur > best) {
>-          bx=x;
>-          by=y;
>-          best = cur;
>-      }
>+        if (cur > best) 
>+          bx= x, by= y, best= cur;

Comma operators group things that need to be grouped. It is a foolish
style that keeps splitting things to separate lines where a misplaced
or missing if bracket will seriously damage the required behaviour.

If you are going to program in "C" learn to use "C" is a good motto.

>9.  As others have pointed out, your handling of dir_ok and
>straightest_direction seems to be less than ideal.  I don't see how you
>could have made them any uglier than they already are, though.

The existing version is a flavour of the imbed-data-in-code style that
leads to lots of cruft and unobvious code to change.

The change computes a proper 8 index position and then at least explicitly
transforms it to the current direction system with one step to understand.

This transformation should be more global or flexible though.

And I am not proud of this, but don't currently have a better way to 
handle the constant DIR_DX to DIR_DX2 switchovers, and the code needs to
be generalized to handle proper map topologies. This can be dropped to the
next round though and another crack taken at it by someone else based
on the corecleanup_06b flavour,

>10.  In at least one place you inadvertently add back in the comment /*
>if the direction is N, S, E or W, the 1 bit is on */.  This was removed
>by my dir-patch1, presumably you didn't integrate cleanly.

Oops ...

>11.  In server/gotohand.c you have the comment
>
> /* This was a really ugly loop calling myrand till one hit a best.
>  * How many bests need we randomly pick from on average, 1???
>  * Now we count them and scan starting at the first known one.
>  */
>
>However, the new loop you've added in is slower than the old one and IMO
>uglier.  You have to look through an average of 4 directions to find the
>appropriate random "best" direction.  The old loop looked through an
>average of just over 4 directions in the worst case, but if there was
>more than one "best" direction it would look through fewer.  There is no
>added "safety" to using this bounded loop since we know there will be at
>least one "best" direction, and the loop doesn't have the elegance that
>Trent's random neighbor search did.

The old loop was unbounded.

The loop counts from the first of the current set, not the beginning of
the loop, and only until it hits the myrand(bc)'th value which if there 
are 2 in question can be 0 or 1 times, so the average is significantly 
less than 4. 

The lower bound is immaterial, it is counting the number of dups in 
general, and has nothing to do with the conditions of Trent's 
rand_adjacent algorithm.

The current flavour calls myrand() once, the loop spin is probably 
negligible.

The current flavour doesn't do any of this if there was only one value.

In short, not one of these comments is really valid.

>12.  In server/gotohand.c L1084 you stop using variable k as a
>directional iterator (good) but then use it to store a continent ID
>(bad!).  Why not use a more legible variable instead?  Elsewhere "ucont"
>is used.

No problem. I don't think k is used for anything else so the extra rename
step is fine.

>13.  In server/settlers.c L260 you have
>
>+      if( i == 2 && j == 2 )
>+        continue;
>
>These magic numbers should be replaced by an appropriate constant (most
>likely CITY_MAP_SIZE/2, possibly a new constant CITY_MAP_CENTER).

The 2's are the original code style here. I agree that they should be
changed to CITY_MAP_SIZE/2 as should all others in this routine.

We probably want to decide how many different MACROS we want to define
for these sort of things, noting that all the constnat folding is done
properly by the compiler as has been pointed out by others.

>14.  It looks like many of your changes in server/settlers.c are purely
>cosmetic???

Actually I touched a lot more of it at one point. Some changes are
likely incomplete backouts that left just formatting updates for things.

For example, I used "dir" rather than "k" for the iterator in one set.

And some are due to indent shifts from *iterate replacements, double
for to single iterate.

IMHO any formatting changes no matter how they get in can only help in
this code area.

>15.  In server/settlers.c lines 1520 and 1560 you change only the
>initial value of variables.  Is this necessary?

I cored with a coordinate of 0x7ffffecd or somthing like that when 
there was no update to gx,gy. This initializes the stack locations to 
something that is expected rather than random stack garbage. View it
as a risk fix - it is what is done in the other routines that call the
same update functions.

>Summary:  This patch should go in ASAP.  All parts of it look good,
>except as noted above.  Most of those fixes can come later in any case;
>this patch fixes so much UGLY cruft that it's well worth the
>shortcomings.  The only parts I'm not sure of are the client GUI code
>and server/settlers.c.
>
>[1] No assumptions should be made about unreal coordinates.  The calling
>function may be calling normalize_map_pos just to wrap the coordinates,
>but this is the incorrect usage!  Special handling is always needed for
>unreal coordinates.  The current drawing system may be broken - what it
>should do is use the absolute coordinates to determine where to draw,

Note that normalize_map_pos does not touch either x or y values of unreal
coordinates, and thus they are passed through in such GUI code. It isn't
the usual way one thinks of using normalize_map_pos, but it does respect
the original flavour and comments, with the added fix that x is left alone.

I think one *really* needs a crash course in GUI tricks to really understand
this, but I think that a GUIcleanup will be needed shortly if not already.

>and use the normalized coordinates to determine what to draw; unreal
>coordinates get special handling.  If this were the case then the map
>should be able to wrap around on the screen (which it can't), so I
>assume things are a bit broken here.  You may have to leave out the
>checks for this reason, but you should put in a comment to that effect
>(which you have done in some of the situations).  In particular, the
>comment around line 1415 of client/gui-gtk/mapview.c makes me believe
>things are handled wrongly now by preserving unreal coordinates and
>hoping that things "just work" later when they get drawn as black.
>
>jason short

Apart from the abuse of subjective terms like ugly which we should all
probably strive to suppress, it looks like you've beat on me and the
code pretty successfully. Thanks - I think.

Cheers,
RossW