[Freeciv-Dev] review of corecleanup_06

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

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

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!

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

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

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]

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.

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.

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;

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.

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.

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.

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.

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

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

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

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