[Freeciv-Dev] Re: Corecleanup patch updates

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Tue, 07 Aug 2001, rwetmore@xxxxxxxxxxxx wrote:
> I've dropped an update in ftp.freeciv.org incoming to last week's
> corecleanup patch that fixes a few bugs and is made against
> 1.12.0-beta-3 aka 1.11.10.

I have some comments:

1) This seems to have your ship movement and mapgen stuff in it.  That
   should be in a separate patch.

2) I'm still not sold on the DIR_D{X,Y}2 stuff.  What is the advantage
   of using them, as opposed to the old ones?

3) Formatting: use 2 spaces for indentation; including in
   find_city_beach().

4) Be aware that when you replace for loops with adjc_iterate you are
   no longer looping over the centre tile.  This causes bugs in
   e.g. ai_manage_barbarian_leader().

5) Do not peek at the variables that are used to implement any of the
   _iterate macros in the code that uses them since doing so defeats
   the purpose of the excercise.  In other words, if you need to know
   what direction you're going in you should use adjc_dir_iterate()
   rather than adjc_iterate() and peek(_nn).

6) What is DIR_BITR() ?  A more descriptive name would be good.

7) FC__MAP_C is not necessary.  AFAIK there is no harm in declaring
   them as extern in map.h, and then defining them in map.c.

8) You dropped an assert(is_real_tile(x,y)) in find_route().

9) I'm not optimistic about reading values from void_tile.  IIRC we
   have had void_tile clobberation bugs in the past, and in the long
   term we ought to get rid of it entirely.

10) I don't think the use of adjc_iterate() in reset_move_costs() is
    appropriate, since memcpy() is rather ugly.  I think it would be
    better to leave as-is here.

11) Don't be afraid to change strange and cruel formatting to be in
    line with Freeciv's style (K&R, two spaces for indentation) if you
    happen to be touching some code.  This is particularly relevant
    for mapgen.c.  However, don't do it if you're not touching
    anything in the vicinity, since that only obscures the important
    portions of your patch.

Can you make another patch that incorporates these suggestion?

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
Yow!  I'm having a quadraphonic sensation of two winos
 alone in a steel mill!