[Freeciv-Dev] Re: Reproducable core dump (PR#1051)

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

 --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote: 
> On Mon, Nov 26, 2001 at 01:04:55PM +0000, Gregory Berkolaiko wrote:
> > 
> > AFAIK 1051 deals mostly with code from gotohand.c
> > If nobody is looking at it yet, you can give it to me.
> 
> Go ahead.

Below is the report on your patch.

It seems to be correctly achieving what it strives to achieve.  It
simplifies the code quite a bit but there are some issues which I would
like to raise:

1. I was told that using goto is a very bad style.  You can get rid of
them quite simply but you don't.  Why not?

2. This piece of code is used by human palyers (AI doesn't fly) but it
peeks under the FOW absolutely shamelessly.  You don't attempt to fix it.
Why not?

3. You use direction vectors to check that you are going in the right
direction.  This is a correct but not the most transperent and fast way. 
I would propose to choose the direction basing on whether we get closer
to the aim or not.  The "direction choice" cycle becomes

  best = real_map_distance(x, y, dest_x, dest_y) + 1;
  adjc_dir_iterate(x, y, nx, ny, dir) {
    int value, dir_dx, dir_dy;
      
    if (is_non_allied_unit_tile(map_get_tile(nx, ny), pplayer)) 
      continue;
    }
      
    value = real_map_distance(nx, ny, dest_x, dest_y);
      
    if (value < best) {
      best = value;
      best_dir = dir;
    }
  } adjc_dir_iterate_end;

You get rid of few lines and variables outside the cycle as well.  You
may use straightest_direction as the tie-breaker if you wish.

To summarise, the patch is good but not ready yet.

Best,
G.

__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page from News and Sport to Email and 
Music Charts
http://uk.my.yahoo.com